Differential D10617

Fix "ambiguous shortcut" issue introduced with D10314
ClosedPublic
Actions

Authored by ngraham on Feb 17 2018, 9:15 PM.

Details

Reviewers
markg
elvisangelaccio
Group Reviewers
Frameworks
Dolphin
Commits
R241:c2a80e71b39d: Fix "ambiguous shortcut" issue introduced with D10314
Summary

My patch D10314 was insufficiently tested (sorry about that!) and introduced an "Ambiguous Shortcut" warning, because "show/hide inline previews" was already bound to F11.

This patch solves that issue by changing the shortcut to F12. F11 was never appropriate for it in the first place, since that's used for something different in Dolphin anyway. Dolphin doesn't currently have a shortcut for "show hide inline previews", so there's nothing to be consistent with. I used F12 in the file dialogs since it's unused in both places, so we could add it to Dolphin too if we really wanted.

Test Plan

Tested with Kate. F11 toggles the aside preview, and F12 toggles the inline preview, like they should. No more conflicts!

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Feb 17 2018, 9:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 17 2018, 9:15 PM
ngraham requested review of this revision.Feb 17 2018, 9:15 PM
ngraham edited the summary of this revision. (Show Details)Feb 17 2018, 9:16 PM
ngraham edited the test plan for this revision. (Show Details)
markg accepted this revision.Feb 17 2018, 9:23 PM

Looks good to me.

This revision is now accepted and ready to land.Feb 17 2018, 9:23 PM
Closed by commit R241:c2a80e71b39d: Fix "ambiguous shortcut" issue introduced with D10314 (authored by ngraham). · Explain WhyFeb 17 2018, 9:37 PM
This revision was automatically updated to reflect the committed changes.
elvisangelaccio added a comment.Feb 17 2018, 10:53 PM

Would be nice to get more feedback before choosing a shortcut that affects many applications.

I don't think that F12 is a good choice. The F keys are few and some of them are already taken (Help, Rename, Reload, Fullscreen - and yes, it's bad that dolphin uses F11 for another thing).
We should try not to use them for "global" shortcuts, but instead let each application choose what they want to do with them.

Do we really need a shortcut for the inline preview in the first place? If we do, let's find something else...

ngraham added a comment.Feb 17 2018, 11:33 PM

The shortcut won't affect other apps since the file dialog its its own context. Even if the host app uses F12 for something, the file dialog will grab the key first, so there's no "ambiguous shortcut" issue.

That said, I'm not wedded to F12 by any stretch of the imagination, and I think removing it entirely wouldn't be the worst thing in the world either.

elvisangelaccio added a comment.Feb 18 2018, 10:45 AM
In D10617#208513, @ngraham wrote:

The shortcut won't affect other apps since the file dialog its its own context. Even if the host app uses F12 for something, the file dialog will grab the key first, so there's no "ambiguous shortcut" issue.

Yes, you won't get the ambiguous shortcut dialog but it'd be a bad idea to use F12 for two different things within the same app (the file dialog is part of the app).

markg added a comment.Feb 18 2018, 12:21 PM
In D10617#208703, @elvisangelaccio wrote:
In D10617#208513, @ngraham wrote:

The shortcut won't affect other apps since the file dialog its its own context. Even if the host app uses F12 for something, the file dialog will grab the key first, so there's no "ambiguous shortcut" issue.

Yes, you won't get the ambiguous shortcut dialog but it'd be a bad idea to use F12 for two different things within the same app (the file dialog is part of the app).

If it were F11 (which it was) then this patch would be very intrusive as you could have potentially broken fullscreen support (with F11), but it isn't it's F12..
Note that you can in fact see this "broken fullscreen". Open gwenview, open an image and press F11, then do CTRL+O to open the file open dialog. Now F11 only has effect on that file open dialog.

But for F12, i don't see anything wrong. As far as i'm aware it isn't bound to any really crucial actions (i would call fullscreen crucial aka F11).
F12 in chrome opens the developer console.
F12 in konsole prints the tilde sign (there is another dedicated key for that, right under escape)

F12 seems fine to me. Yes, you'd get into the weird issue that F12 in the app might have a different behavior in a file open dialog opened by the very same app, but that is with every key you choose.
As long as we are consistent in keys which i thing Nate is very much trying to do (good job!).

Also, note that F11 in kwrite binds to something completely different (show line numbers).

Revision Contents
Changeset List

Diff 27430

View Options

src/filewidgets/kdiroperator.cpp

Loading...
View Options

src/filewidgets/kfilewidget.cpp

Loading...