Toggling fullscreen mode in Gwenview does not reflect in window manager
and vice versa.
This patch changes toggling fullscreen mode from setWindowState() to
KToggleFullScreenAction::setFullScreen() as the api doc recommends.
BUG: 195046
Details
Details
Start Gwenview in windowed or fullscreen mode and toggle by window
manager, e.g. Alt+F3 → More Actions → Fullscreen.
Use other tools to toggle Gwenview's fullscreen mode:
kstart --fullscreen gwenview image.jpg wmctrl -i -r <WINID> -b add,fullscreen wmctrl -i -r <WINID> -b remove,fullscreen
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
Maybe some of the mFullScreenMode bools could be removed now from the child widgets and check the application fullscreen mode instead.
Btw. a fullscreen start page can be reached by kstart --fullscreen gwenview or setting a shortcut for "start page" and pressing this after switching to fullscreen mode. But colors are not adjusted.
This patch does not fix session restore in any way (https://bugs.kde.org/show_bug.cgi?id=387782). Fullscreen mode, viewed image / browsed folder, etc. needs to be (re)stored.
But should not be too hard to do according to the following docs and the basic calls are already in main().
https://techbase.kde.org/Development/Tutorials/Session_Management
http://doc.qt.io/qt-5/session.html
Comment Actions
Can you perhaps connect to QWindow::windowStateChanged and show/hide the toolbars and so on?
Comment Actions
The MainWindow is not derived from QWindow, but I can override QWidget::changeEvent() to get these events.
Do you see any problems with the actual toggleFullScreen() slot or is this just a cleaner or simpler way?
Comment Actions
@broulik Could you detail what you were referring to? I'm as confused as Peter.
@muhlenpfordt You submit patches faster than I can review them ;)
Tested this (also in IceWM), works as advertised. I'll try to review the code later.
Interesting, I didn't now this trick until now.
This patch does not fix session restore in any way (https://bugs.kde.org/show_bug.cgi?id=387782). Fullscreen mode, viewed image / browsed folder, etc. needs to be (re)stored.
Are you sure? For me it works better than before:
- Color scheme restored.
- Correct special fullscreen toolbar.
- Exiting fullscreen now restores normal mode.
The only fullscreen related problem still open is that the window decoration is missing in normal mode after switching back in case of a restored session.
Unrelated to your patch and unrelated to fullscreen mode:
- Remembering the image already worked.
- Browse/View mode not remembered yet.
Comment Actions
I'll take a pause... someday. ;) No hurry for review.
Are you sure? For me it works better than before:
You're right - after setting up the same versions of gwenview bin and lib in my test VM it looks really better. :)
Comment Actions
Thanks again. Ready for the next one ;)
| app/mainwindow.cpp | ||
|---|---|---|
| 132–133 | Please remove. Also I wonder why we store a lot of things in the config, but use a transient variable here and there (grep for BeforeFullscreen). Perhaps for a future patch there is some opportunity for refactoring, i.e. store sidebar/toolbar/statusbar/thumbnailbar status in the config (where it makes sense separately for normal/fullscreen mode) and just use it from there? | |
| app/mainwindow.cpp | ||
|---|---|---|
| 132–133 | Should I just remove the whole MainWindowState and rename this lonely bool to mToolBarVisibleBeforeFullScreen (or any prettier name)? Or let's keep this for a cleanup patch? | |
Comment Actions
I have a patch which removes all of them (except for kcfg and command line args), I'll post it once I made sure everything still works.
| app/mainwindow.cpp | ||
|---|---|---|
| 132–133 | I guess fiddling with mToolBarVisible in a bunch of places makes the patch harder to read, so doing it later is probably more sensible. In principle you are right, though ;) | |
Comment Actions
That's really the last question now, just to be sure. ;)
This one goes to master or is it safe enough for stable?
Comment Actions
I'm not sure myself. It fixes a BUG:, but we don't really know whether someone might rely on the window manager provided mechanism which had slightly different results. This means we change the behaviour a bit indeed. So maybe only to master?