Review Request 123508: Shortcuts broken when user sets secondary shortcut

[Lindsay Roberts]

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

(Updated May 2, 2015, 9:04 a.m.)


Status
------

This change has been marked as submitted.


Review request for KDE Frameworks and David Faure.


Changes
-------

Submitted with commit c7b1b1b89e388779356a5dcde291d6e6eac7705b by Lindsay Roberts to branch master.


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

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