fix default shortcut detection
ClosedPublic

Authored by sitter on Oct 24 2019, 9:39 AM.

Details

Summary

When the default shortcut is empty the UI string "None" is used to
indicate that. Alas, when mapping QKeySequences to the UI label we have to
rely on QKS::toString being equal/notequal to the text in the label and
that is not the case since an empty QKS will be an empty string, but
our display string is "None".

This resulted in the "custom" sequence detection incorrectly thinking all
shortcuts without a default are customized when in fact they aren't, the
strings are just different because we express it as "None" and QKS as "".
To mitigate this we'll simply ensure empty sequences are serialized to our
None reference string instead of an empty string.

Test Plan

broken master:

with fix:

also tested continued working of:

  • setting a custom key
  • unsetting a custom key

Diff Detail

Repository
R263 KXmlGui
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Oct 24 2019, 9:39 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 24 2019, 9:39 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Oct 24 2019, 9:39 AM
sitter edited the test plan for this revision. (Show Details)Oct 24 2019, 9:40 AM
sitter added a reviewer: dfaure.
aacid added a subscriber: aacid.Oct 24 2019, 6:43 PM

Can this be autotested?

Supposedly, I was being lazy though since there is no pre existing test and I don't exactly know if there's anything to watch out for when testing widgets (as in: I've never written a widget test before).

aacid added inline comments.Oct 24 2019, 10:02 PM
src/kshortcuteditwidget.cpp
72–73

this is wrong, isn't it?

Or at least behaviour changing, before m_defaultLabel was set to None, when defaultText was empty. Now it's not?

sitter added inline comments.Oct 24 2019, 10:21 PM
src/kshortcuteditwidget.cpp
72–73

it's initialized to None via the ctor at line 68 already. now that I notice it, defaultText could also be const now.

aacid accepted this revision.Oct 24 2019, 10:42 PM

ok, fair enough about the tests

src/kshortcuteditwidget.cpp
72–73

ah right.

This revision is now accepted and ready to land.Oct 24 2019, 10:42 PM
This revision was automatically updated to reflect the committed changes.