Quitting Gwenview was saving the current state of the Thumbnail Bar, which is always
hidden in fullscreen, and therefore was always saving an OFF state to the config.
This patch saves the state of the Thumbnail Bar before going to fullscreen.
Details
- Reviewers
rkflx - Group Reviewers
Gwenview - Commits
- R260:75f1996cb3f4: Fix thumbnail bar in View resetting to OFF when quitting in fullscreen
Go to View, toggle Thumbnail Bar on, go to fullscreen, press Ctrl+Q to quit.
After launching Gwenview again, View Thumbnail Bar should be visible.
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.
I guess your patch did no lack proper testing and you just uploaded the wrong revision :D
Even when your test plan contains a weird case, the most basic function should work too: Open Gwenview, toggle Thumbnail Bar, quit.
I wonder if we can get rid of mThumbnailBarVisibleBeforeFullScreen? In fullscreen mode we always hide the thumbnailbar, and in normal mode it could just directly read the config value (see also a similar refactoring Peter did recently in c2e8c8923841). Not sure if it will work, though, because the menubar checkbox has to be in sync.
app/viewmainpage.cpp | ||
---|---|---|
460 | As far as I can see this is only called on startup and when you close the config dialog, i.e. too seldom ;) |
Apologies, I was too hasty in submitting this patch. I swear I tested the above though...
I wonder if we can get rid of mThumbnailBarVisibleBeforeFullScreen? In fullscreen mode we always hide the thumbnailbar, and in normal mode it could just directly read the config value (see also a similar refactoring Peter did recently in c2e8c8923841). Not sure if it will work, though, because the menubar checkbox has to be in sync.
I've updated it (and hopefully tested thoroughly enough!), to work like this:
- The checked state of the thumbnail bar action is used to track what the user wants
- Fullscreen sets the visibility of the bar directly, without toggling the action
- When saving the config, we use the checked state of the action
What this means is that if the thumbnail bar is visible in normal mode, going to fullscreen will hide it but the button/action will remain checked to indicate the state the user wants it in (which fullscreen overrides). In Normal mode, they should stay in sync.
Therefore in a way, the checked state of the action is replacing mThumbnailBarVisibleBeforeFullScreen.
Not sure whether this is a 'good' way of doing it.
Works great now, thanks. Removing yet another bool sometimes getting out of sync is just what I had in mind. Makes the code easier to understand, too.
Some small cleanup left, then we should be good to go.
Apologies, I was too hasty in submitting this patch. I swear I tested the above though...
No problem, that's what reviews are for.
app/viewmainpage.cpp | ||
---|---|---|
164–165 | Still there ;) | |
460–462 | I would just repeat the call to the static, I don't think the temporary bool gains us anything in terms of performance or readability. |