Clean up code tracking fullscreen state
ClosedPublic
Actions

Authored by rkflx on Feb 9 2018, 5:44 PM.

Details

Reviewers

Commits

R260:140393d08972: Clean up code tracking fullscreen state

Summary

After syncing Gwenview's fullscreen mode with that of the window manager
in 0d917337122f, we can just ask the window for its fullscreen state
instead of having to track this for ourselves.

This gets rid of a bunch of bools and reduces chances those might
accidentally get out of sync at some point. In addition, we can
eliminate duplication and dependence on execution order in
MainWindow::toggleFullScreen.

Test Plan

qDebug() comparing mFullScreenMode and window()->isFullscreen()
Manual testing of every functionality affected:
- Switching to and from fullscreen mode (both via Gwenview and window manager).
- ⎋ to exit fullscreen mode.
- Special fullscreen toolbar works.
- Toggling sidebar resizes fullscreen toolbar.
- Sidebar state remembered separately for normal and fullscreen mode.
- Savebar not visible in fullscreen mode.
- Memory warning bar still works in fullscreen mode.

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.

rkflx requested review of this revision.Feb 9 2018, 5:44 PM

rkflx created this revision.

rkflx edited the test plan for this revision. (Show Details)Feb 9 2018, 6:09 PM

Looks good and I can't find any difference to previous behaviour. :)
Strange that savebar takes account into fullscreen mode but is never visible here.

Only two issues I found while testing, but entirely independent of this patch:

If statusbar is visible in windowed view mode it's hidden when switching to fullscreen. You have to press F3 two times to show it there. Without looking closer I suspect it's just hidden but an internal state is not updated.
Sidebar visibility is a bit irritating, since view mode saves separate windowed/fullscreen states but browse mode's state is linked for windowed and fullscreen.

app/fullscreencontent.cpp
343	I asked myself why `mContent` returns the correct window since it gets no parent on construction - it seems the layout creates this link, so everything ok. :)

This revision is now accepted and ready to land.Feb 10 2018, 10:04 AM

In D10412#203839, @muhlenpfordt wrote:

Looks good and I can't find any difference to previous behaviour. :)

Thanks for the review!

Strange that savebar takes account into fullscreen mode but is never visible here.

It is visible when you rotate a couple of huge images on a machine with a small amount of RAM:

Working on Gwenview I frequently encounter new hidden gems ;)

Only two issues I found while testing, but entirely independent of this patch:

If statusbar is visible in windowed view mode it's hidden when switching to fullscreen. You have to press F3 two times to show it there. Without looking closer I suspect it's just hidden but an internal state is not updated.

Yeah, I've noticed that too a few weeks back, but never got around to file the bug…

Sidebar visibility is a bit irritating, since view mode saves separate windowed/fullscreen states but browse mode's state is linked for windowed and fullscreen.

Hmh, we should sort that out.

Closed by commit R260:140393d08972: Clean up code tracking fullscreen state (authored by rkflx). · Explain WhyFeb 11 2018, 11:39 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			app/browsemainpage.cpp (5 lines)
M			app/fullscreencontent.h (1 line)
M			app/fullscreencontent.cpp (7 lines)
M			app/mainwindow.cpp (27 lines)
M			app/savebar.cpp (15 lines)
M			app/viewmainpage.h (2 lines)
M			app/viewmainpage.cpp (8 lines)