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

Improved window maximizing #426

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Improved window maximizing #426

merged 1 commit into from
Jul 20, 2023

Conversation

akiyosi
Copy link
Owner

@akiyosi akiyosi commented Dec 5, 2022

Resolves problems associated with maximizing windows with WindowGeometryBasedOnFontmetrics setting enabled.
Related to #373

@damanis
Copy link

damanis commented Dec 5, 2022

@akiyosi
If you solving the problem with maximize, may be you check if it is easy add support for horizontal and vertical maximize, too?

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 5, 2022

@damanis
Hmmm, that doesn't sound easy.
There is no such window state in Qt's namespace either.
https://doc.qt.io/qt-6/qt.html#WindowState-enum

@damanis
Copy link

damanis commented Dec 6, 2022

@akiyosi
Yes, I see that Qt does not provide proper maximize flags. May be, because the flags do not exist in Windows?
In Linux (freedesktop standard) 'xprop' shows tree available states.

_NET_WM_STATE(ATOM) = *_NET_WM_STATE_MAXIMIZED_HORZ
WM_COMMAND(STRING) = { "/tmp/neovim/nvui/bin/nvui", "--geometry=99x44", "--" }
_NET_WM_ALLOWED_ACTIONS(ATOM) = _NET_WM_ACTION_MOVE, _NET_WM_ACTION_RESIZE, _NET_WM_ACTION_MINIMIZE, _NET_WM_ACTION_SHADE, _NET_WM_ACTION_STICK, _NET_WM_ACTION_MAXIMIZE_HORZ, _NET_WM_ACTION_MAXIMIZE_VERT, _NET_WM_ACTION_FULLSCREEN, _NET_WM_ACTION_CHANGE_DESKTOP, _NET_WM_ACTION_CLOSE

This is output for 'nvui', another neovim GUI that uses QT too.
I few find in its code, seems, it does nothing to support horizontal and vertical maximization, but it work.

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 9, 2022

@damanis
I have tested briefly with the following steps to verify that window sizes are restored before and after horizontal maximization.

  1. Enable WindowGeometryBasedOnFontmetrics in settings.toml

  2. Launch goneovim with some option for this test

    goneovim -c "set columns=40 lines=10" -c "set titlestring=testwindow" 
    
  3. Confirmation of horizontal maximization

    wmctrl -r testwindow -b add,maximized_horz
    
  4. Confirm that the original window size is restored

    wmctrl -r testwindow -b remove,maximized_horz
    

@damanis
Copy link

damanis commented Dec 9, 2022

@akiyosi
I did some test as you wrote - Goneovim window didn't restore after horizontal maximize.
I use this version: https://github.com/akiyosi/goneovim/actions/runs/3620712059#artifacts
('set titlestring' also don't work in this version, but the window title is set by wmctrl).

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 11, 2022

@damanis
I wonder why....... It works fine in my Pop!OS environment.

('set titlestring' also don't work in this version, but the window title is set by wmctrl).

Sorry, It needs to set set title command.

./goneovim -u NONE -c "set title" -c "set titlestring=testwindow"

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 16, 2022

@damanis
I was able to reproduce the problem you noted and found the cause.
However, this is a difficult problem to solve...

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 17, 2022

@damanis
I have made some changes to allow restoring the original window size for horizontal and vertical maximization. It works fine in my environment, but it would be helpful if you could test it in yours.

https://github.com/akiyosi/goneovim/actions/runs/3718473010

@damanis
Copy link

damanis commented Dec 17, 2022

@akiyosi
I tried the provided image. Full maximization works properly, horizontal and vertical don't work.
I see your fix add some timers, may be it relates to system performance? Tomorrow I will try the image on faster system.

I was able to reproduce the problem you noted and found the cause.

Could you explain short what the cause?

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 18, 2022

@damanis

Could you explain short what the cause?

  • In previous implementations of this PR(a4e1204), in settings where WindowGeometryBasedOnFontmetrics is enabled, adjustSizeBasedOnFontmetrics() is executed after each window resizing operation to resize the window based on fontmetrics.

  • On the other hand, as a matter of fact, multiple window resizing operations occur when a maximization event occurs, since window maximization is represented by an animation in a typical OS.

    It seems that the reason window size restoration does not work well after horizontal or vertical maximization is that the adjustSizeBasedOnFontmetrics() resizing process that occurs during the window resize event just before the window size reaches its maximum size overwrites "the window size before maximization" may stored in the window system prior to the maximization.

    The reason the full-maximization process works well is that it is possible to detect the occurrence of the event in Qt's window state change process only for full-maximization, so that full-maximization process can be handled accordingly internally.

The change idea for e571b41 is to set a timer upon the occurrence of each window resize event to execute a single adjustSizeBasedOnFontmetrics() after 200msec. If multiple resize events occur intermittently, these resize events will reset the count of the existing timer to 200msec.
I don't know why it doesn't work well in your environment, but it is possible that the count value of the timer is not reasonable for your environment.

@damanis
Copy link

damanis commented Dec 18, 2022

@akiyosi
On second system the provided image doesn't start, due to libc6 version (2.31 on Ubuntu 20.04). On this system I can't build goneovim manually.

@damanis
Copy link

damanis commented Dec 18, 2022

@akiyosi

The reason the full-maximization process works well is that it is possible to detect the occurrence of the event in Qt's window state change process only for full-maximization, so that full-maximization process can be handled accordingly internally.

I'll try check if possible get window state directly from NET_WMSTATE (on Linux).

@damanis
Copy link

damanis commented Dec 25, 2022

@akiyosi
Could you decrease goneovim dependencies to be compatible with Ubuntu 20.04 Focal (glibc >=2.31б etc.)?
Or, is it possible to provide full static goneovim binary?

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 25, 2022

@damanis
I will consider providing binaries.
Note that the change in the required version of GLIBC is due to the various tools that are integrated into the Runner of Github Actions being upgraded.

https://github.com/actions/runner-images

Therefore, a straightforward way to do this would require downgrading the tools used in Runner, and I do not have a concrete working image.

@akiyosi
Copy link
Owner Author

akiyosi commented Dec 25, 2022

@damanis
How about this binary?
goneovim-test-426-linux.tar.gz

@damanis
Copy link

damanis commented Dec 25, 2022

@akiyosi
The provided test build works.

@damanis
Copy link

damanis commented Dec 25, 2022

@akiyosi

Note` that the change in the required version of GLIBC is due to the various tools that are integrated into the Runner of Github Actions being upgraded.

It has support for Ubuntu 20.04 (End of Standard Support - April 2025).

@akiyosi
Copy link
Owner Author

akiyosi commented Jul 8, 2023

@damanis
Hey, thanks for your comments on the issue #373 (comment).
I have been working on this issue for a bit of a period of time, so I would like to summarize the current status of this issue and the remaining issues.

Are the following perceptions correct?

Problem

The following problems exist with window maximization (more generally, window state changes) when WindowGeometryBasedOnFontmetrics is enabled.

  • Unable to restore window size after restoring full-maximization window.
  • Unable to restore window size after restoring vertical/horizontal-maximization window
    • This has not been solved. The main reason is that it is not possible to handle horizontal and vertical maximization in layers of Qt

@damanis
Copy link

damanis commented Jul 9, 2023

@akiyosi
Checked Improved window maximizing #1123: full maximize/unmaximize works.

I think, vertical/horizontal-maximization can be solved as I wrote in #373.
It is not clear for me, why Goneovim change size on RESIZE event - nvim-qt does nothing.

@akiyosi
Copy link
Owner Author

akiyosi commented Jul 9, 2023

@damanis
Thanks for the comment!
I will try your idea.

It is not clear for me, why Goneovim change size on RESIZE event - nvim-qt does nothing.

This is because when WindowGeometryBasedOnFontmetrics is enabled, goneovim resizes the window size in fontmetrics units on the resize event. I understand that this issue does not occur whenWindowGeometryBasedOnFontmetrics is disabled.

@akiyosi
Copy link
Owner Author

akiyosi commented Jul 10, 2023

@damanis
Hey, I have checked your idea;

  1. add struct to store current and previous window size struct { current_size; previous_size; maximized: bool; }
  2. in resize handler copy current_size to previous_size (in addition to current code)
  3. in maximize handler set maximized to true
  4. in next resize or unmaximize restore window size to previous_size if maximized is true. Set maximized to false.

And it is essentially equivalent to what I am currently implementing in the improved-window-maximize branch.

The reason this implementation is not effective for horizontal and vertical maximization is that, as mentioned above, the maximized flag cannot be set to true in process 3. in your idea, because horizontal and vertical maximization cannot be detected in Qt's layers.

@damanis
Copy link

damanis commented Jul 11, 2023

@akiyosi
Vertical and horizontal maximization are other NET_WM states that full maximization.
Does Qt not call maximize callback when window maximized, for example, vertically? In this case Goneovim should not know what exactly maximization is used.
Of course, this issue not critical, but theoretical.

@damanis
Copy link

damanis commented Jul 16, 2023

@akiyosi
I tested version 14-Jul-2023 with WindowGeometryBasedOnFontmetrics = false - all three maximization types works.

@akiyosi
Copy link
Owner Author

akiyosi commented Jul 18, 2023

@damanis
Yes, this problem is specific to the case where WindowGeometryBasedOnFontmetrics is true.

@akiyosi akiyosi force-pushed the master branch 4 times, most recently from 9bf414c to 1760226 Compare July 19, 2023 09:01
@akiyosi
Copy link
Owner Author

akiyosi commented Jul 20, 2023

@damanis
I have no good idea handling horizontal and vertical maximization. On the other hand, I would like to merge this PR because the current fix can improve the maximization behavior.
Do you have any suggestions?

@damanis
Copy link

damanis commented Jul 20, 2023

@akiyosi
I think, merge and choose this issue is a good idea.
With WindowGeometryBasedOnFontmetrics = false all work properly.

@akiyosi akiyosi merged commit 64460f7 into master Jul 20, 2023
@akiyosi akiyosi deleted the improve-window-maximize branch July 20, 2023 23:41
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