Review Request 123508: Shortcuts broken when user sets secondary shortcut

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123508/#review79764
-----------------------------------------------------------


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.

- David Faure


On May 1, 2015, 9:17 a.m., Lindsay Roberts wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123508/
> -----------------------------------------------------------
> 
> (Updated May 1, 2015, 9:17 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.h fa2d7fb 
>   autotests/kxmlgui_unittest.cpp 404d304 
>   src/kxmlguifactory.cpp aeeb242 
> 
> Diff: https://git.reviewboard.kde.org/r/123508/diff/
> 
> 
> Testing
> -------
> 
> Primarily using trunk Konsole - saving/loading/using multiple/single shortcuts.
> 
> 
> Thanks,
> 
> Lindsay Roberts
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150501/ed952c6b/attachment-0001.html>