| rkflx | |
| muhlenpfordt |
| Gwenview |
If one opens the config with a keyboard shortcut in fullscreen mode, changes something,
and saves it, the color palette reverts to Normal (non-fullscreen), most noticeably changing
the background color.
This fix checks the fullscreen state when applying color palettes.
Change config while in fullscreen - colors should stay the same.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
The background color in browse mode is ok, but in view mode it is changed (even if config is changed in browse mode an then switching to view mode).
Also the button colors change on browse page:
Also fix issues in Image View fullscreen: - Palette resetting to normal - Status bar reappearing
Thanks @huoni. I'll let @muhlenpfordt do the review and landing for this one (of course I'm available for any questions or if you want a final check).
@muhlenpfordt Not sure how I messed that up, but it's fixed now and ready for another review. Thanks!
The color changes are fixed, so far looks good. :)
Better add "color" to the title to be more precise what this patch does.
@rkflx: Are you working on a fix for the F3 status bar issue in fullscreen view mode? If not I'll look for it.
| app/browsemainpage.cpp | ||
|---|---|---|
| 275–276 | I would use a temp const QPalette variable here to be more readable (but just a suggestion) . ;) | |
| app/viewmainpage.cpp | ||
| 504 | Why are you completely disabling the status bar in view mode? Maybe because it's getting visible after config change? | |
Made the title a bit more descriptive.
| app/viewmainpage.cpp | ||
|---|---|---|
| 504 | Without this, after saving config in fullscreen Image View mode, the status bar reappears, because in loadConfig() it's activated. It doesn't disable it complete, just for fullscreen. I wanted to put this check close to the code that decides that the status bar should be hidden in fullscreen mode in the first place, and that is just below on line 510. | |
Feel free to work on this, I'm glad if I can make progress with my backlog instead of putting more stuff on top of it ;)
I think you can ignore the status bar bug here and remove the fullscreen check in ViewMainPage::setStatusBarVisible() (and update Summary/Test plan accordingly). I'm working on the fix to keep the KToggleAction and status bar visibility in sync. That will prevent making the status bar visible here and the user can still press F3 to show it.
Totally unrelated but to keep the tradition of finding at least one more bug while fixing another... ;)
I expected to open an image in fullscreen view after clicking on the icon below but it's still the browse mode in fullscreen.
Totally unrelated but to keep the tradition of finding at least one more bug while fixing another... ;)
I expected to open an image in fullscreen view after clicking on the icon below but it's still the browse mode in fullscreen.
Oh noes, another item uncovered from my secret list of Gwenview improvements! Just joking, send a patch ;) This has been bothering me for a long time. I think it was meant to be more of a feature than a bug, because you can reach View by double-clicking. Nevertheless it does not make any sense to have a gazillion buttons doing the same thing when we even have a separate button in the toolbar. We could think about making it configurable or whether View or Slideshow is better, but I'd tend to just go with View for simplicity.
I think you can ignore the status bar bug here...
No worries, removed and updated the summary to focus on the colors.
Looks good to me now. Many thanks. :)
@rkflx Any objections? If I 'm to land this, is arc patch + arc land enough to preserve author info?
Trying arc land --hold I'm asked to "Land this revision by huoni?" which sounds promising. ;)
Ok, here we go on the topic of landing Diffs on behalf of someone:
If patches are uploaded via the web form (check by hovering over the first timestamp), you'd have to use git commit --amend --no-edit --author "name <name@domain>". However, @huoni made is easy for you by using Arcanist, meaning you can skip this step. Still you should always check the author field (just look at git log), because it might contain bogus information like "abc@localhost" if misconfigured locally.
Here typing arc land and pressing y will suffice.
| app/browsemainpage.cpp | ||
|---|---|---|
| 277 | @huoni Technically you are changing from NormalPalette to NormalViewPalette, even if visually it doesn't make a difference (because there's only a difference in Fullscreen, i.e. the grainy background, and for going there setFullScreenMode is called anyway where it is set again). Still, I'd prefer doing it the correct way so that loadConfig and setFullScreenMode work the same (someone else might change things in the future, and suddenly your code breaks…). | |
| app/browsemainpage.cpp | ||
|---|---|---|
| 277 | Ah that makes sense. May I ask what the difference is with the View versions of the palettes. When should one be used over the other? | |
| app/browsemainpage.cpp | ||
|---|---|---|
| 277 | I've kind of answered my own question. I can see now that the only difference in the View versions of the palettes is the background (both) and foreground/text color (Normal only). | |
| app/browsemainpage.cpp | ||
|---|---|---|
| 277 | As far as I've understood, *Palette is for setPalette, and *ViewPalette is for mThumbnailViewPalette, which means you should do it like this: setPalette(d->mGvCore->palette(window()->isFullScreen() ? GvCore::FullScreenPalette : GvCore::NormalPalette)); d->mThumbnailView->setPalette(d->mGvCore->palette(window()->isFullScreen() ? GvCore::FullScreenViewPalette : GvCore::NormalViewPalette)); Maybe it would make sense to add a comment to where the enum is defined? At least I was confused at first (wrongly assumed it was about Browse vs. View mode). Looking at the code, we see that the View variants are used for DocumentViewContainer as well as ThumbnailView, and the non-View variants for all the rest. | |
| 399–400 | I believe this is how it's supposed to look. Here's the difference between both. | |
Thanks for checking @rkflx, I missed the difference. :(
Maybe we should create something like applyPalette() as in ViewMainPage to keep this in a single place?
Don't worry about it ;)
Maybe we should create something like applyPalette() as in ViewMainPage to keep this in a single place?
@huoni Sounds great.
@huoni Well done, thanks again. Looks so much better now. Obligatory plug: Bug 390331.
@muhlenpfordt Thanks for the review and good luck with landing ;)
Sure thing. Thanks for being my subject for landing on behalf. ;)
Result in git looks good to me.