akiyosi / goneovim

A GUI frontend for neovim.
MIT License
2.41k stars 63 forks source link

Improved window maximizing #426

Closed akiyosi closed 1 year ago

akiyosi commented 1 year ago

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

damanis commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

akiyosi commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@damanis

Could you explain short what the cause?

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 commented 1 year ago

@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 commented 1 year ago

@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 _NETWMSTATE (on Linux).

damanis commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

damanis commented 1 year ago

@akiyosi The provided test build works.

damanis commented 1 year ago

@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 commented 1 year ago

@damanis Hey, thanks for your comments on the issue https://github.com/akiyosi/goneovim/issues/373#issuecomment-1626090901. 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.

damanis commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

akiyosi commented 1 year ago

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

akiyosi commented 1 year ago

@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 commented 1 year ago

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