Fix shortcut config changes sometimes not updating until restart
ClosedPublic

Authored by muhlenpfordt on Apr 7 2018, 4:27 PM.

Details

Summary

After setting or resetting custom shortcuts for actions in Browse or
Start Page Mode and then switching to View Mode these changes are not
active until next Gwenview restart.
This patch adds a refreshActionProperties() call after the shortcut
config dialog is closed to fix this.

BUG: 389331
FIXED-IN: 18.04.0

Test Plan
  • Start Gwenview in Browse Mode or Start Page Mode
  • SettingsConfigure Shortcuts...
  • Set custom shortcut for Quit to e.g.
  • Switch to View Mode displaying any image
  • should quit Gwenview
  • Restart Gwenview in Browse Mode or Start Page Mode
  • SettingsConfigure Shortcuts...
  • Reset shortcuts with Defaults
  • Switch to View Mode displaying any image
  • Ctrl+Q should quit Gwenview

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.Apr 7 2018, 4:27 PM
muhlenpfordt created this revision.

On one of my systems (Kubuntu 17.10) this bug happens only sometimes, so much for Comment 1. On two other it's always reproducible (Kubuntu 17.10 and 16.04).

I found this solution when reading the comment for KXMLGUIFactory::refreshActionProperties():

This is needed, for example, when you change shortcuts scheme at runtime.

PS: Shouldn't Phabricator show a branched from Applications/18.04?

muhlenpfordt edited the summary of this revision. (Show Details)Apr 7 2018, 5:00 PM

Removed unnecessary semicolon

huoni added a subscriber: huoni.Apr 7 2018, 9:49 PM

I can't reproduce this on 18.04 or 17.12.3 (my system version), so I'm afraid I can't test.

PS: Shouldn't Phabricator show a branched from Applications/18.04?

I would have thought so too.git diff Applications/18.04 only shows this revision's changes, so it should be fine?

rkflx added a subscriber: rkflx.Apr 7 2018, 10:49 PM

PS: Shouldn't Phabricator show a branched from Applications/18.04?

Yeah, I noticed this too: Only in some cases it says "branched from master", but sometimes this is missing entirely, not sure why. At least gitk --all correctly shows that you intend this patch for the stable branch.

rkflx added a comment.Apr 7 2018, 11:05 PM

PS: Shouldn't Phabricator show a branched from Applications/18.04?

Yeah, I noticed this too: Only in some cases it says "branched from master", but sometimes this is missing entirely, not sure why. At least gitk --all correctly shows that you intend this patch for the stable branch.

Ah, got a suspicion: Did you use git checkout -b <name> or arc feature <name> (which does the checkout for you, but also calls --set-upstream-to=…)?

Ah, got a suspicion: Did you use git checkout -b <name> or arc feature <name> (which does the checkout for you, but also calls --set-upstream-to=…)?

I used the first git command. I'll try with arc feature next time.

rkflx accepted this revision.Apr 8 2018, 4:37 PM

Patch (and bug) with the longest steps to reproduce ever, but this seems to fix it. Thanks!

I can't reproduce this on 18.04 or 17.12.3 (my system version), so I'm afraid I can't test.

I had to issue gwenview to begin with the Start Page, otherwise the bug would not trigger for me.

app/mainwindow.cpp
481–483

Looks like this was quite tricky to figure out… (But I cannot find anything better.)

This revision is now accepted and ready to land.Apr 8 2018, 4:37 PM
muhlenpfordt added inline comments.Apr 9 2018, 7:46 AM
app/mainwindow.cpp
481–483

Looks more complicated than it is and seems to be the only way (apart from SLOT) until KActionCategory offers a new syntax addAction(). ;)

This revision was automatically updated to reflect the committed changes.
rkflx added inline comments.Apr 9 2018, 8:53 AM
app/mainwindow.cpp
481–483

If you know how to, you could send a patch for KActionCategory ;)