Previous versions of gwenview used ⎋ to exit fullscreen mode.
By changing shortcut handling of ⎋ this does not work anymore.
This patch adds an eventFilter() to leave fullscreen from browse
mode with ⎋.
BUG: 305659
Details
Details
- Reviewers
ngraham rkflx - Commits
- R260:1e9a4482d900: Exit fullscreen browse mode with Escape
- Open gwenview in browse mode
- Switch to fullscreen mode (e.g. F11)
- Press ⎋
-> Should switch back to windowed browse mode
Diff Detail
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.
Comment Actions
I tried with different shortcut configurations for ⎋:
- ⎋ assigned to browse mode (default)
- ⎋ assigned to other action (e.g. Quit)
- ⎋ not assigned to any shortcut
There's one thing that may lead to confusion - if ⎋ is assigned to e.g. Quit, you need to press twice from fullscreen browse mode to quit Gwenview.
| app/mainwindow.cpp | ||
|---|---|---|
| 1251 ↗ | (On Diff #26182) | resetAutoSaveSettings() does not disable saving of window state and after quitting Gwenview in fullscreen mode, it restarts maximized. |
Comment Actions
Fantastic! This bug was a source of frustration for my wife, so I'm very happy to see it getting fixed. :) I'll test it out and offer a formal review later today.
I think this is perfectly acceptable FWIW.
Comment Actions
Actually, I tested this with Esc bound to Quit Gwenview, and the quit command overrode the exit fullscreen command. It was a little jarring. I would prefer the behavior you mentioned above.
Comment Actions
From fullscreen browse mode ⎋ should switch to windowed browse mode. Only after the second ⎋ Gwenview should quit. If anything else happens, there's a bug in my code.
In fullscreen view mode actually there is no shortcut override (https://bugs.kde.org/show_bug.cgi?id=305659#c7). It's no problem to switch to windowed view mode instead of browse mode (or whatever action ⎋ is bound to). I vote for this behavior, too. ;)
Comment Actions
Excellent, works great. Just a couple of questions regarding details of the code, nothing serious.
As far as I've understood, most users wanting to assign ⎋ in such a way are trying to emulate Quick Look, probably meaning no Fullscreen mode is involved (unless used with -f). This is fine IMO as there is apparently no easy way to solve it properly.
That's exactly what I get. I guess Nate was in the other mode.
In fullscreen view mode actually there is no shortcut override (https://bugs.kde.org/show_bug.cgi?id=305659#c7). It's no problem to switch to windowed view mode instead of browse mode (or whatever action ⎋ is bound to). I vote for this behavior, too. ;)
Here we are only adding something, not changing existing behaviour. Your suggestion occured to me in the past too, but in the end I quite like how it works currently: ⎋ always goes to Browse mode, no matter if Fullscreen or not. Consistency is important. We should not take that away from users used to this right now. Also, it makes the Fullscreen Browse mode a bit more discoverable. Besides that, we still have F11 and Enter for choosing between both variants explicitly.
| app/browsemainpage.cpp | ||
|---|---|---|
| 79 | I suspect Qt, KF5 or Gwenview is already tracking the fullscreen state. Can we find a way to avoid replicating this here (a stray bool in the code easily gets out of sync…)? See also the other comment, as well as how KToggleFullScreenAction is used in the code. | |
| 284 | Normally this is done via mFullScreenAction->trigger. What if we kept MainWindow::leaveFullScreen? This even has the fullscreen check built-in. (Your current approach works fine, of course. Just wondering if we could solve the bool issue and whether we might avoid introducing subtle inconsistencies.) | |
| app/mainwindow.cpp | ||
| 1251 ↗ | (On Diff #26182) | Good find, again, but I wonder whether this should be in a separate patch. Quit is only a test case, but really we are working on Browse and Fullscreen modes here, aren't we? Any reason you did not use saveConfigGroup? As far as I know, setAutoSaveSettings has an overload for KConfigGroup. |
| app/browsemainpage.cpp | ||
|---|---|---|
| 79 | The fullscreen mode check is needed before accepting the event, otherwise this would override the ⎋ action in windowed browse mode. | |
| 284 | It's not really needed to remove the separate action or MainWindow::leaveFullScreen(), so let's keep it. This way the shortcut is still configurable. | |
| app/mainwindow.cpp | ||
| 1251 ↗ | (On Diff #26182) | Maybe too obvious as it's just one line away... ;) |
| app/browsemainpage.cpp | ||
|---|---|---|
| 79 | Better safe than sorry, but okay, I give in as there's a precedent already (missed that fact somehow, oops) and it isn't wise to reinvent the wheel. | |
Comment Actions
Awesome. It would be great to land this before Sunday, so I can announce it in my weekly Usability & Productivity round-up along with several other Gwenview fixes. The more of them we can announce in the same post, the more momentum Gwenview will seem to have, and the higher likelihood of getting more contributors!