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.

In D10853#214226, @rkflx wrote:

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.

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 ;)

In D10207#199517, @muhlenpfordt wrote:

Maybe a commandline option --nofullscreen like kstart supports is a solution? For me, that's all I need. ;)

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.

In D10853#214238, @rkflx wrote:

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.

+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. :)

In D10853#214263, @muhlenpfordt wrote:

I could also look for adding fullscreen mode for the start page (new diff of course).

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…

In D10853#214344, @rkflx wrote:

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…

Absolutly no problem. It should keep on the normal users track not a private one. ;)

In D10207#199517, @muhlenpfordt wrote:

Maybe a commandline option --nofullscreen like kstart supports is a solution? For me, that's all I need. ;)

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.

In D10853#214344, @rkflx wrote:

Therefore, could we simply add this to kstart?

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).

In D10853#214572, @muhlenpfordt wrote:

In D10853#214344, @rkflx wrote:

Therefore, could we simply add this to kstart?

Tested with kstart4 --nofullscreen - it's not a 'prevent fullscreen' but a 'do not enter fullscreen' aka 'do nothing' option

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 D10853#214263, @muhlenpfordt wrote:

Btw. the listed hidden option ShowLockZoomButton seems to be more than hidden - it's nonexistent ;)

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.

In D10853#215810, @rkflx wrote:

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.

What do you think about the draft in P172 to solve the memleak warning?

In D10853#216105, @muhlenpfordt wrote:

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.

In D10853#216109, @muhlenpfordt wrote:

What do you think about the draft in P172 to solve the memleak warning?

LGTM