Re: Review Request 123508: Shortcuts broken when user sets secondary shortcut
--- 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() == qMetaTypeIdQListQKeySequence () 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
Re: Review Request 123508: Shortcuts broken when user sets secondary shortcut
--- 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. Changes --- Removed unnecessary type match. 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() == qMetaTypeIdQListQKeySequence () 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 (updated) - 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
Re: Review Request 123508: Shortcuts broken when user sets secondary shortcut
On May 1, 2015, 9:24 p.m., David Faure wrote: The conditional comes from 50a2283112225e2db4673e41d12411e8664865fa in kdelibs.git where qMetaTypeIdKShortcut was ported to qMetaTypeIdQListQKeySequence . 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() == qMetaTypeIdQListQKeySequence () 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
Re: Review Request 123508: Shortcuts broken when user sets secondary shortcut
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123508/ --- (Updated April 28, 2015, 8:43 p.m.) Review request for KDE Frameworks. Changes --- Added autotest under kxmlgui_unittest. 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() == qMetaTypeIdQListQKeySequence () 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 (updated) - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 123508: Shortcuts broken when user sets secondary shortcut
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123508/ --- (Updated April 27, 2015, 6:01 p.m.) Review request for KDE Frameworks. Changes --- Clarified summary. Summary (updated) - Shortcuts broken when user sets secondary shortcut 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() == qMetaTypeIdQListQKeySequence () 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 - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 123508: kxmlgui: Fix load of shortcuts when alternate set.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123508/ --- Review request for KDE Frameworks. 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() == qMetaTypeIdQListQKeySequence () 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 - 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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
