> On May 1, 2015, 9:24 p.m., David Faure wrote: > > The conditional comes from 50a2283112225e2db4673e41d12411e8664865fa in > > kdelibs.git where qMetaTypeId<KShortcut> was ported to > > qMetaTypeId<QList<QKeySequence> >. However that porting looks wrong indeed, > > KShortcut was UserType in QVariant while, from what you say, the property > > is now just one QKeySequence, and QKeySequence is a builtin QVariant type. > > > > So it seems to me that you can remove the old conditional and just use > > yours instead. That works in your tests, right? > > No point in keeping possibly-wrongly-ported unused code.
Ah that's what it's from. Thankyou for solving that mystery. Confirmed the fix without the old condition, removed it. - Lindsay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123508/#review79764 ----------------------------------------------------------- On May 2, 2015, 12:55 a.m., Lindsay Roberts wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/123508/ > ----------------------------------------------------------- > > (Updated May 2, 2015, 12:55 a.m.) > > > Review request for KDE Frameworks and David Faure. > > > Bugs: 337131, 339243, 340803, and 345411 > https://bugs.kde.org/show_bug.cgi?id=337131 > https://bugs.kde.org/show_bug.cgi?id=339243 > https://bugs.kde.org/show_bug.cgi?id=340803 > https://bugs.kde.org/show_bug.cgi?id=345411 > > > Repository: kxmlgui > > > Description > ------- > > When a user configures both a primary and alternate shortcut for an action, > they were lost on subsequent load of the app/part .rc file. > > These values are persisted in the .rc file as two key sequence strings > separated by "; " -- the format understood by `QKeySequence::listFromString`. > When these files are reparsed, the current execution flow simply calls > `QObject::setProperty("shortcut", semicolonSeparatedString)`, which is > `Q_PROPERTY` bound in QAction to `setShortcut(QKeySequence)` -- invoking the > (single) QKeySequence constructor. The embedded semicolon and secondary > shortcut seem to break this constructor. > > There is another specific flow in `KXMLGUIFactoryPrivate::configureAction`; > one that specifically calls > action->setShortcuts(QKeySequence::listFromString(attribute.value())); > > but it was conditionalised by: > > propertyType == QVariant::UserType && > action->property(attrName.toLatin1().constData()).userType() == > qMetaTypeId<QList<QKeySequence> >() > > I have not been able to track down in history when that conditional would've > run true for the QAction property `"shortcut"`, QVariant::KeySequence was > added Nov 2011, and the conditional has been there since at least the > monolithic git split Dec 2013. > > Nor have I been able to track down any recent changes on the Qt side that > would've implied the QKeySequence string constructor would've worked for > multiple shortcuts in the recent past. > > In any case, the fix is simply to add a second conditional - matching on > QVariant::KeySequence. > > > Diffs > ----- > > autotests/kxmlgui_unittest.cpp 404d304 > src/kxmlguifactory.cpp aeeb242 > autotests/kxmlgui_unittest.h fa2d7fb > > Diff: https://git.reviewboard.kde.org/r/123508/diff/ > > > Testing > ------- > > Primarily using trunk Konsole - saving/loading/using multiple/single > shortcuts. > > > Thanks, > > Lindsay Roberts > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
