Currently Gwenview always starts in normal (non-fullscreen) mode
independent of its state on previous quit.
This patch saves the fullscreen mode and restores it on next start.
BUG: 383093
rkflx |
Gwenview |
Currently Gwenview always starts in normal (non-fullscreen) mode
independent of its state on previous quit.
This patch saves the fullscreen mode and restores it on next start.
BUG: 383093
Quit Gwenview in normal/fullscreen mode and check if it restarts in
previous mode.
Commandline option --fullscreen overrides the saved state.
Lint Errors | Excuse: check later |
Severity | Location | Code | Message |
---|---|---|---|
Error | app/main.cpp:112 | Cppcheck | memleak |
No Unit Test Coverage |
I thought about a three state option instead of the on/off switch. Any preferences for this version?
Space saving version | ... or similar to other radio buttons |
TBH, only looking at the screenshot I don't understand what the feature is about. I had to read the commit message to understand it. How can we expect normal users to get this? And how would they even quit Gwenview from fullscreen if they don't know keyboard shortcuts? For always-fullscreen they might be irritated as well.
Also, is there any other app which has a similar GUI config option? We could take a look at how they phrase it, but if there is none with a similar target user base as Gwenview perhaps this is an indication we should not add this feature.
As indicated in previous discussions, I'm not convinced at all about this feature (sorry).
Maybe make it a hidden config option? But then again, I think this is for power users and those can already simply use -f.
@ngraham Thoughts?
And as for D10207#199393, I have some notes for a new task to improve consistency across apps (not done yet, sadly).
Hmh, my previous comments were not all that clear. Let me try again:
I think we should define a sane standard across apps (whatever that is). Running away by burdening the user with the decision is not what I want, though.
Personally I prefer having more options to configure every application to my needs - this is a main reason why I use KDE. ;)
But I really can't find any other software offering such a switch, so maybe we'll adapt to that.
Keeping it as hidden option does not make much sense. (Btw. the listed hidden option ShowLockZoomButton seems to be more than hidden - it's nonexistent ;) ).
If nobody loves this config option I remove the dialog part + RememberFullScreenMode but keep the --no-fullscreen commandline option. Ok?
I could also look for adding fullscreen mode for the start page (new diff of course).
It's not easy to have to say no to a new feature like that, especially when the patch is already written. But I guess someone has to do this once in a while to keep Gwenview on track. I hope you understand…
Adding the commandline option we'd have to do the same to Okular and Kate, too, to keep consistency. Yes, KDE's software is customizable, but also simple by default and consistent (at least in theory ;)
I cannot confirm that with kstart --help | grep fullscreen.
Therefore, could we simply add this to kstart? I already supports --fullscreen just fine, and I think it's exactly the tool you want at this level of customization. I tried it with Okular (which already supports remembering the last fullscreen state, like I'd imagine it for Gwenview), and it works except for the missing --no-fullscreen (although we might have to come up with better wording).
Could you look into that? Only if there is absolutely no way to make this work I'd grudgingly accept a commandline option for Gwenview.
+1; this is a more general problem, not a Gwenview-specific one. If there's anywhere a settings needs to be added it's proabbly somewhere in System Settings - → Window Management.
A hidden textfile-only config option is almost useless; globally, the number of users who actually use these can probably be counted on the fingers of one hand. :)
Try it, but it's probably a can of worms ;) Perhaps it would make sense to Add url navigator widget to start page first, so you could reuse the top bar from fullscreen Browse mode to have the Leave Fullscreen button. But then you'd have to introduce some kind of start:/ URL, or make it display the start page when going up from /. Hmh, better open a task first with some design ideas before starting to implement this…
Absolutly no problem. It should keep on the normal users track not a private one. ;)
I cannot confirm that with kstart --help | grep fullscreen.
This was a feature of the KDE4 KCmdLineOptions and was lost in migrating to QCommandLineParser. So it does not work with kstart5.
I also thought that boost supports this but can't find any evidence.
Therefore, could we simply add this to kstart? I already supports --fullscreen just fine, and I think it's exactly the tool you want at this level of customization.
Ok, that's maybe the better place for this. I'll take a look at it.
Tested with kstart4 --nofullscreen - it's not a 'prevent fullscreen' but a 'do not enter fullscreen' aka 'do nothing' option: Gwenview switches to fullscreen anyway.
I.e. loosing this option in kstart5 is effectively not a loss. ;)
But e.g. a KWin rule to prevent fullscreen on program init does it - so there's a way.
Phew. Thanks for being considerate. Will review as soon as https://phabricator.kde.org/D10844 is handled (feel free to give a hand if you want).
Now I see what you mean. I tested kstart{3,4,5} --help, and all of them do not have --nofullscreen. You simply refer to the automatic negation of options, which does not work anymore. I don't think it ever had an effect unless an application specifically supported it.
In 2014, 5572aded5dfe moved the feature to the GUI. Looking at the history of the wiki page, there was not that much change since 2012. I simply removed it from the page now.
Thanks for helping out with the other reviews, this really makes my life easier.
Tried the patch, looked at the code. I think you considered everything and it can go straight to arc land ;)
memleak
Anything to worry about? Perhaps it' worth trying out Valgrind or ASAN, might solve some of the random crashes on Bugzilla…
cppcheck complains about the Gwenview::MainWindow* window which is not deleted. Maybe it could be moved to a member variable of StartHelper and correctly destructed.
Seems like mMainWindow was intended for this but that one is never used.
Nice catch. I thought about creating it on the stack, but that clashes with the else for kRestoreMainWindows.
LGTM