Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set a default size on first launch #1100

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Conversation

davvid
Copy link
Contributor

@davvid davvid commented Feb 28, 2024

When ~/.config/nvim-qt/window-geometry.conf does not exist then
restoreGeometry() will initialize a window that is tiny and must be
resized in order to be usable in some environments.

Resize the window to half the screen when launched for the first time.
Do not restore window state when it is invalid or missing.

@davvid davvid force-pushed the initial-size branch 2 times, most recently from 7ec3ee4 to 4e638d7 Compare February 28, 2024 08:42
@jgehrig
Copy link
Collaborator

jgehrig commented Feb 29, 2024

This looks closely related to #1096. Let's apply these changes in a single PR over there.

Also the hard-coded size concerns me, it might not be the best size for all platforms.

A few comments:

  1. Does the annoying behavior go away when 1096 is applied?
    I suspect when the geometry is not explicitly restored, the window manager may set a reasonable default size. This is worth testing and discussing quickly on all major platforms (Windows, Linux, MacOS).
  2. We should avoid the hard-coded value of QSize{ 1280, 1440 }. Maybe a QSetting can be used?

If we really do need this feature think this feature is necessary, it should likely be an optional setting in window-geometry. Maybe something like default_window_size?

@davvid
Copy link
Contributor Author

davvid commented Mar 2, 2024

Sounds good. I'll revisit this after working through #1096 since it's touching the same part of the code.

  1. Does the annoying behavior go away when 1096 is applied?

When I first wrote this patch I was in a temp branch where I had the 1096 changes in effect and it didn't make a difference. I'm going to re-test just to be sure.

  1. We should avoid the hard-coded value of QSize{ 1280, 1440 }

I can understand that. There's also a part of me that thinks that it's 2024 and the vast majority of users will be just fine with that value, but I agree that we can do better.

This will typically only come into effect on the very first run (e.g. if you rm -r ~/.config/nvim-qt you can see this behavior) so a QSetting option to control this doesn't seem that helpful. It's a one-time annoyance, but it's also one of those "try to make a good first impression" kind of polish that's worth improving.

Here's an idea.. we could query the screen size using QScreen https://doc.qt.io/qt-5/qscreen.html (I would suggest QDesktopWidget but it's deprecated and removed in Qt6) and then clamp the initial size to the screen size.

Alternatively, instead of clamping, we could just set the size to be the same as the screen size (effectively making the UI take the full screen) or some portion of it (e.g. half width of the screen).

Let me know what you think. I'll get back to #1096 now and get that into shape for merging.

@davvid
Copy link
Contributor Author

davvid commented Mar 2, 2024

Here's what I landed on.

  • When no configuration is present (e.g. on the very first run) then we will apply a default size.
  • When configuration is present and users have asked us to not restore geometry then we will also apply a default size. I suspect that users that are asking for not restoring geometry do not expect the window to appear tiny on every run, hence this behavior.
  • We query for the screen size to determine the default size. For screens in a Landscape orientation we will use half the width of the screeen and full height. For screens in Portrait mode we will use the full width of the screen and half height.

I'm testing on Gnome/Wayland. I suspect that the "tiny initial screen size" issue is seen on other platforms as well, so this should be sensible behavior on all platforms.

Most users will not change defaults. What they'll get is the initial size that we're setting here and then whatever size and position they set manually will be preserved, so there's probably few users who won't want the default size.

If users don't like the default size they can always use the (enabled-by-default) behavior where we restore window geometry.

I hope we don't need another QSettings entry because the window_geometry field already serves that purpose.

Let me know what you think. Hopefully this is a nice approach.

@davvid
Copy link
Contributor Author

davvid commented Mar 2, 2024

This PR extends #1096 so I'll rebase this after that topic is resolved. Only a single commit will remain in this branch post-rebase.

@davvid davvid force-pushed the initial-size branch 2 times, most recently from d7cd261 to bf2cce2 Compare March 5, 2024 08:27
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
@jgehrig
Copy link
Collaborator

jgehrig commented Mar 6, 2024

Everything looks good! A few small comments added. Sorry to be nit-picky :)

I like the high-level approach and this looks Qt6 compatible (if I'm reading the Qt docs correctly).

However, I think we should make one behavior change regarding this:

When configuration is present and users have asked us to not restore geometry then we will also apply a default size. I suspect that users that are asking for not restoring geometry do not expect the window to appear tiny on every run, hence this behavior.

I suspect the users who want the geometry restore off, will also NOT want us poking the window size. Let's make it so when restore_window_geometry == false we don't write QSettings values or modify the window size on startup.

Some window managers provide a reasonable default size or do their own size restoration. Also, tiling window managers force a window size. Here is an example Issue: #679

@davvid
Copy link
Contributor Author

davvid commented Mar 7, 2024

Thanks for the explanation and review ~ not nitpicky at all. I've consted this up and re-pushed.

While we can't make SetDefaultSize() take const QWidget& (QWidget::resize() is non-const), there's no need for a separate function now that we'll only be using it in a single place. I've inlined the guts of SetDefaultSize() directly into restoreWindowGeometry() instead.

@davvid davvid changed the title mainwindow: set a sensible default size Set a default size on first launch Mar 7, 2024
@davvid davvid force-pushed the initial-size branch 3 times, most recently from f602849 to 1476b87 Compare March 7, 2024 05:36
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
src/gui/mainwindow.cpp Outdated Show resolved Hide resolved
@jgehrig
Copy link
Collaborator

jgehrig commented Mar 20, 2024

@davvid Sorry for the long delay -- I was away from my computer.

Two small comments -- The approach looks great to me, let's get this merged!

When `~/.config/nvim-qt/window-geometry.conf` does not exist then
restoreGeometry() will initialize a window that is tiny and must be
resized in order to be usable in some environments.

Resize the window to half the screen when launched for the first time.
Do not restore window state when it is invalid or missing.
@davvid
Copy link
Contributor Author

davvid commented Mar 21, 2024

Good stuff, thanks for the re-review. This should be good to go now.

@jgehrig jgehrig merged commit 442abfb into equalsraf:master Mar 21, 2024
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants