Disable auto saving window size in fullscreen mode
ClosedPublic

Authored by muhlenpfordt on Jan 31 2018, 4:59 PM.

Details

Summary

If gwenview quits while in fullscreen mode the window size is
automatically saved and gwenview restarts in maximized state.
This patch disables saving window state before switching to
fullscreen mode.

Test Plan
  1. Open gwenview in windowed, not maximized mode
  2. Switch to fullscreen mode (e.g. F11)
  3. Quit gwenview by Ctrl+Q
  4. Restart gwenview

-> Window size should be equal to step 1 and not maximized

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.
muhlenpfordt requested review of this revision.Jan 31 2018, 4:59 PM
muhlenpfordt created this revision.

Bugfix separated from D10182.

rkflx accepted this revision.Jan 31 2018, 5:14 PM

Nice and simple ;) Yesterday I tried to find an existing bug for this, but could not find anything so far.

To me the patch sounds like material for the stable branch, i.e. Applications/17.12.

This revision is now accepted and ready to land.Jan 31 2018, 5:14 PM
This revision was automatically updated to reflect the committed changes.

I think there is an easier way to do this, but arc land --onto... wanted to include a couple of commits from master.
This is what I did now:

$ git checkout fullscreen-nosave-window-size
$ git merge-base origin/master HEAD
-> 899423ad41611b94848ed8fbdc877bd49f9cd408
$ git rebase --onto 'Applications/17.12' 899423ad41611b94848ed8fbdc877bd49f9cd408 fullscreen-nosave-window-size
-> now my local source dir was equal to Applications/17.12 branch, except my single modification
$ arc land --hold --onto 'Applications/17.12' 
$ git push -- origin 74509b4af0009f818c288659bde16ae81e6afdf5:Applications/17.12

$ git checkout master
$ git merge -Xours origin/Applications/17.12
$ git push

The result looks ok to me. :)

rkflx added a comment.Feb 1 2018, 8:27 PM

The result looks ok to me. :)

Yup.

I think there is an easier way to do this, but arc land --onto... wanted to include a couple of commits from master.

Good thing you checked before ;). Just tried to find an easier way, but sadly there is none! Seems like arc land does not support rebasing the way I thought it would. The --onto argument only specifies the name of a remote branch (e.g. if no remote tracking branch had been set in Git before). It is assumed you do the rebasing yourself – unless you want to land all commits from the local branch which are not yet on the remote. However, for commits exclusive to master that's obviously not what we want.

Also I'm sorry my tips were quite vague regarding the rebase. I think you've figured it out, though. I just played around a bit myself to reduce assumptions about the current local state even further. This is what I'll now add to my playbook (let me know if you think there are any flaws):


# rebase feature-branch from master to the most up-to-date stable branch
git checkout feature-branch
git fetch
git rebase --onto origin/Applications/17.12 master

After this, continue working/testing until it is time to land:

# land feature-branch (which has been branched from stable) on the remote stable branch

# Option 1:
git checkout -b new-feature-branch --track origin/Applications/17.12
arc land --preview

# Option 2:
git branch --set-upstream-to=origin/Applications/17.12
arc land --preview
 
# Option 3:
arc land --onto Applications/17.12 --preview

For completeness: Instead of the convenient arc land you could also do those steps manually:

arc amend
git push
git checkout Applications/17.12
git pull
git branch -D feature-branch

(↑ Once we worked out all kinks we should add this to the wiki some time or other, I case you are wondering why I wrote so much again…)

rkflx added a subscriber: ngraham.Feb 1 2018, 10:25 PM

(Submitted already, but subscribing the promo dept. for visibility ;) )

See, this is why we need a Gwenview project! I could become a watcher and you wouldn't need to CC the promo department. :)

rkflx added a comment.Feb 1 2018, 10:38 PM

IIRC when we last talked about this in November I was not against having such a thing. It's only that I'm not making any promises wrt. how much time I can spend, so I won't push this too much from my side. If you want to go ahead, I'm not stopping you ;)

ngraham added a comment.EditedFeb 1 2018, 10:38 PM

Does this patch bear any relationship to https://bugs.kde.org/show_bug.cgi?id=383093?

rkflx added a comment.EditedFeb 1 2018, 11:09 PM

Is there any relationship of this change to https://bugs.kde.org/show_bug.cgi?id=383093?

Interesting, missed this one as I was searching for "fullscreen", but not "full-screen".

I guess the reporter wants to Quit, and after restarting Gwenview should be Fullscreen again immediately. Before the patch we started maximized (but not Fullscreen), now we start with the same window size as before going Fullscreen.

Checking what other apps are doing:

  • Firefox: does not start fullscreen
  • Chromium: does not start fullscreen
  • LibreOffice: does not start fullscreen
  • Konsole: same bug as Gwenview before the patch
  • Kate: fullscreen
  • Okular: fullscreen
  • QtCreator: fullscreen

Not sure what to do now… I think it's nice if you are stuck in fullscreen and don't know how to get out of it again to close and recover. On the other hand the bug reporter might have a point. I would tend to restoring the fullscreen state, as the other problem should be solved by other means.

Still, Peter's patch is right for the moment (phew!), because the Start Page is not available in Fullscreen. I guess we should change that (promo FTW!?) which would be more consistent anyway, and then fix Gwenview and Konsole to behave like Kate and Okular.

@muhlenpfordt @ngraham Thoughts?

That sounds like a sensible plan to me. Consistency with our own apps seems important.

If we do that, we might consider making it even easier to exit full screen mode for people who feel stuck: https://bugs.kde.org/show_bug.cgi?id=385314

Not sure what to do now… I think it's nice if you are stuck in fullscreen and don't know how to get out of it again to close and recover. On the other hand the bug reporter might have a point. I would tend to restoring the fullscreen state, as the other problem should be solved by other means.

I would not restore fullscreen state automatically (or at least with a config option), but definitely fix the window manager fullscreen-signal for gwenview. This way a user can check the "Remember fullscreen state" in window manager settings if this feature is really wanted (it's a bit weird IMHO ;) ) and perhaps the session restore will be fixed, too.

rkflx added a comment.Feb 2 2018, 11:05 AM

Seems like Nate put his finger on something handled very inconsistently between apps and it's not really clear what the best behaviour would be. I'll do more testing and open a task targetting consistency across KDE's applications shortly.

"Remember fullscreen state" in window manager settings

I assume you mean "Special Window Settings"? If so, I have doubts about that, because it is application specific or at least quite fiddly to apply exactly to all apps where you want to. I think we should default to one of the variants (I'd prefer restoring fullscreen), the special option should still work for overriding that.

it's a bit weird IMHO ;)

Well, Kate's and Okular's developers disagree, and those are quite high-profile apps in our stable :D

at least with a config option

Any suggestion where we could place such an option globally in systemsettings? (But be prepared for quite some backlash, because in general providing a sane default is preferred over having yet another option, as you can even override this right now as you pointed out).

fix the window manager fullscreen-signal for gwenview
perhaps the session restore will be fixed

That would be great, regardless of what comes out of the other discussion.

I assume you mean "Special Window Settings"? If so, I have doubts about that, because it is application specific or at least quite fiddly to apply exactly to all apps where you want to. I think we should default to one of the variants (I'd prefer restoring fullscreen), the special option should still work for overriding that.

Yes, that's what I mean. But I thought of setting this for gwenview only, not globally. Also only a gwenview config option.
Maybe a commandline option --nofullscreen like kstart supports is a solution? For me, that's all I need. ;)

Sorry for not answering your last question in time :/

I just saw it as a TODO in my notes for the topic.

Related: Always-fullscreen would be problematic as long as the startpage does not support fullscreen, wouldn't it?