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

handle CSI resize events #3785

Closed
wants to merge 4 commits into from

Conversation

williamspatrick
Copy link

No description provided.

Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
@williamspatrick
Copy link
Author

This might be a little rough, but it works. I welcome any feedback you have and I'll make improvements as suggested.

I feel like I'm missing one minor operation in the low level resize handling code. When I issue a resize, the OS window resizes as expected, but if I click lower-right as if to resize it snaps back to the original size.

This can be tested with:

resize () {
	case $1 in
		(mutt) __resize_to 65 80 ;;
		(irc) __resize_to 100 120 ;;
		(default) __resize_to 50 80 ;;
		(small) __resize_to 25 80 ;;
		(*) __resize_to $1 $2 ;;
	esac
}

__resize_to () {
	printf "\e[8;$1;$2;t"
}

One minor flaw is that when a tab-bar is present it ends up resizing to 1 character shorter than requested due to the tab-bar. By this I mean I see the following behavior:

$ resize 25 80 && stty size           
24 80

@williamspatrick
Copy link
Author

I feel like I'm missing one minor operation in the low level resize handling code. When I issue a resize, the OS window resizes as expected, but if I click lower-right as if to resize it snaps back to the original size.

I'm still investigating this, but it is even worse than I wrote here.

When the window loses focus the xdgToplevelHandleConfigure is getting an event where the width and height are still the old values.

@kovidgoyal
Copy link
Owner

On Wayland IIRC programs are only allowed to resize their windows during
mouse events and they have to provide the serial of the event. SO I
doubt this will ever work robustly. Not to mention tiling window
managers where it wont work at all. As such I dont think this feature is
worth implementing, just for some X11 window managers.

@kovidgoyal kovidgoyal closed this Jul 2, 2021
@williamspatrick
Copy link
Author

williamspatrick commented Jul 2, 2021

On Wayland IIRC programs are only allowed to resize their windows during mouse events and they have to provide the serial of the event. SO I doubt this will ever work robustly.

There was a missing call to xdg_surface_set_window_geometry. I've added this and now the code works perfectly under X11 and Wayland.

Not to mention tiling window managers where it wont work at all.

Agree it will not do anything useful for tiling window managers. It is quite useful for non-tiling window managers as there is currently no other way to resize the OS window. (ctrl-shift-r will only resize windows within a layout, unless I miss something.)

As such I dont think this feature is worth implementing, just for some X11 window managers.

I've tested this on X11, Wayland, and MacOS now. It works on all 3 except on MacOS it ends up with stty size being 2x what is desired. I suspect I'm missing a HiDPI scaling factor somewhere in kitty/window.py:resize_from_escape.

I've repushed my latest changes to https://github.com/williamspatrick/kitty/tree/csi-resize , which was where this PR was from. I think if you re-open the PR it will refresh from that branch as well.

@kovidgoyal
Copy link
Owner

Well, I am happy that you got it to work on Wayland, am surprised it does, but good. However, I dont think the CSI escape code is really suitable. Imagine a kitty OS Window with multiple windows inside. If a CSI request to resize one of those windows is received, should the multi-window layout be resized or the OS Window or both/neither?

Incidentally which wayland window manager are you testing it with?

If it does indeed work on wayland, I am willing to consider adding os window resize functionality, but I think rather than the CSI code, a better fit would be remote control, where you can add a kitty @ resize-os-window command alongside the existing resize-window command.

And github doesnt let me reopen this PR so I suggest we settle ona design here and then you can opena new one.

@williamspatrick
Copy link
Author

And github doesnt let me reopen this PR so I suggest we settle ona design here and then you can opena new one.

Sure thing.

Incidentally which wayland window manager are you testing it with?

Gnome 40 / mutter.

Imagine a kitty OS Window with multiple windows inside. If a CSI request to resize one of those windows is received, should the multi-window layout be resized or the OS Window or both/neither?

Right. I recognized this condition and currently detected it.

+        if len(self.windows) != 1:
+            log_error('Unable to resize layout-ed windows.')
+            return
+
+        if window is not self.active_window:
+            log_error('Unable to resize inactive window.')
+            return

What I chose to do right now is drop the CSI request if there are multiple windows. (The if is not active_window might be redundant?)

I think the ideal implementation would be to do some layout calculations so that the sub-window is resized to the CSI amount and the other windows are reflowed per the current layout. But, for the time being I left it as 'neither' until someone enterprising enough to tackle the layout part of it comes around.

gnome-terminal and xterm both support the CSI sequence. If you run something like gnome-terminal+tmux, tmux eats the CSI. To me, 'neither' would mimic the behavior someone might expect coming from that.

It would be trivial to change this to 'OS window' + reflow, but from a user perspective that seems a little surprising so I didn't do it that way. Deleting the above if-conditions would get us to this behavior if you think it is better though.

I think rather than the CSI code, a better fit would be remote control, where you can add a kitty @ resize-os-window command alongside the existing resize-window command.

I can certainly add a resize-os-window as well but, as a user, I really like the CSI codes as well. I spend a fair amount of time connected to remote machines for development and have my dotfiles synchronized between them. It is nice to be able to run something like resize large && htop && resize default and have it do the expected thing no mater which machine I'm on. Without the CSI support I wouldn't be able to resize my local window from a remote server.

I see a few obvious options:

  1. Keep CSI support as is here.
  2. Add an option to enable this CSI support.
  3. Add a kitty @ resize-os-window.

I think that 2+3 is something I could implement in rather short order if that is acceptable to you.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jul 2, 2021 via email

@williamspatrick
Copy link
Author

Would be nice to check if resizing actually works on at least one other window manager.

I'll give something else a try as well. Do you have any suggestions that would be best for more of your users?

You can just piggy back of the existing resize-window command, see rc/resize_window.py

Thanks for the pointer.

remote control works over ssh as well. Though there is the caveat that you need to trust the machines you are SSHing into as remote control over ssh means any program you run on the remote machine can control kitty.

tbh, I've only been using kitty for a few days. Doesn't the remote control over ssh also require a kitty binary to be put on the remote machine? I think that's the bigger hang-up for me.

  1. Implement support for CSI 8 9 and 10 t i.e. resize, maximize and fullscreen

Will do. 9/10 should be trivial.

For reference (https://ttssh2.osdn.jp/manual/4/en/about/ctrlseq.html):

    =  9    Change maximize state of window.
            Ps2 = 0    Restore maximized window.
                = 1    Maximize window.

    = 10    Change full-screen state of window. Currently use the window maximizing instead.
            Ps2 = 0    Undo full-screen mode.
                = 1    Change to full-screen.
                = 2    Toggle full-screen.
  1. CSI 8 t should be "smart": - if there is a single window in the current layout, resize the OS Window - if there are multiple windows, resize the OS Window enough so that the reflowed layout satisfies the resize request as much as possible, and further resize the layout to try to get closer to the requested size

Understood. The single window part is done now. I'll see about the reflow part using the code pointer you gave me above.

  1. Add CSI 88 t and CSI 89 t that resize only OS Window and only window in layout.

Do you have any reference on 88/89 or did you make these numbers up? I'm not seeing any sources.

I would assume these should follow the format of 8.

  1. Add a resize-os-window remote control command

I'm planning to focus on CSI first. Will let you know how much progress I make.

  1. Add an option to turn off window resizing via CSI codes. Report the status of this option via https://sw.kovidgoyal.net/kitty/kittens/query_terminal.html (see how it is done for the allow_hyperlinks option).

Yep.

I realize this may be more work than you are willing to do.

I think most of these are fairly doable.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Jul 2, 2021 via email

@williamspatrick
Copy link
Author

weston and/or kwin would be my choices.

I launched a nested Weston instance and resize works the same as I'm seeing on Mutter.

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