kitty terminal not triggering 'size-changed' on Wayland

TiagodePAlves commented 3 months ago

Another problem with kitty, but this one was harder to pin down the exact problem. I realized that when opening the terminal for the first time, it would never show up. However, if I opened the terminal manually, then hiding and reopening the terminal window would start working correctly. After some testing, I realized that the sizeChangedSignalConnector is never executed, probably because the kitty window doesn't trigger this 'size-changed' signal somehow.

Proposed Solution

My idea to fix this is to check if the size changed synchronously right after resizing the window in _fitTerminalToMainMonitor.

    _fitTerminalToMainMonitor() {
        /* after the main code is run... */

        const frame = window.get_frame_rect();
        // check if the size has already changed synchronously
        const sizeChanged = frame.x === terminalX && frame.y === area.y &&
            frame.width === terminalWidth && frame.height === terminalHeight;
        return sizeChanged;
    }

Then we remove the initial clip if the size is already correct, without waiting for a 'size-changed' signal.

    _adjustTerminalWindowPosition() {
        /* ... */
        const mapSignalHandler = (sig, wm, metaWindowActor) => {
            /* ... */
            const sizeChangedSignalConnector = Util.once(
                this.terminalWindow,
                "size-changed",
                () => {
                    if (this._internalState !== Util.TERMINAL_STATE.RUNNING) {
                        this._internalState = Util.TERMINAL_STATE.RUNNING;
                        this.actor.remove_clip();
                        this._showTerminalWithAnimationTopDown();
                    }
                },
            );

            this._connectedSignals.push(sizeChangedSignalConnector);

            const sizeCorrect = this._fitTerminalToMainMonitor();
            if (sizeCorrect) {
                this._internalState = Util.TERMINAL_STATE.RUNNING;
                this.actor.remove_clip();
                this._showTerminalWithAnimationTopDown();
            }
        }
        /* ... */
    }

Would this solution be okay for most cases?

System Information

cat ~/.local/share/gnome-shell/extensions/quake-terminal@diegodario88.github.io/metadata.json

```json { "_generated": "Generated by SweetTooth, do not edit", "description": "Quickly launch a terminal in Quake mode using a keyboard shortcut", "donations": { "github": "diegodario88", "kofi": "diegodario" }, "name": "Quake Terminal", "settings-schema": "org.gnome.shell.extensions.quake-terminal", "shell-version": [ "45", "46" ], "url": "https://github.com/diegodario88/quake-terminal", "uuid": "quake-terminal@diegodario88.github.io", "version": 19, "version-name": "1.6.2" } ```

inxi -Fxz

```raw System: Kernel: 6.8.5-arch1-1 arch: x86_64 bits: 64 compiler: gcc v: 13.2.1 Desktop: GNOME v: 46.0 Distro: Arch Linux Machine: Type: Desktop Mobo: ASRock model: B450M Steel Legend serial: UEFI: American Megatrends v: P10.08 date: 01/19/2024 Battery: [...] CPU: Info: quad core model: AMD Ryzen 5 1500X bits: 64 type: MT MCP arch: Zen rev: 1 cache: L1: 384 KiB L2: 2 MiB L3: 16 MiB Speed (MHz): avg: 2435 high: 3600 min/max: 1550/3500 boost: enabled cores: 1: 3482 2: 3368 3: 1377 4: 1378 5: 3520 6: 3600 7: 1378 8: 1378 bogomips: 56031 Flags: avx avx2 ht lm nx pae sse sse2 sse3 sse4_1 sse4_2 sse4a ssse3 svm Graphics: Device-1: NVIDIA GM206 [GeForce GTX 960] vendor: eVga.com. driver: nvidia v: 550.67 arch: Maxwell bus-ID: 07:00.0 Display: wayland server: X.org v: 1.21.1.13 with: Xwayland v: 23.2.6 compositor: gnome-shell driver: gpu: nvidia,nvidia-nvswitch resolution: no compositor data resolution: 1920x1080 API: EGL v: 1.5 drivers: nvidia,swrast,zink platforms: active: gbm,wayland,x11,surfaceless,device inactive: device-1 API: OpenGL v: 4.6.0 compat-v: 4.5 vendor: nvidia mesa v: 550.67 glx-v: 1.4 direct-render: yes renderer: NVIDIA GeForce GTX 960/PCIe/SSE2 API: Vulkan v: 1.3.279 drivers: nvidia surfaces: xcb,xlib,wayland devices: 1 Audio: [...] Network: Device-1: Realtek RTL8111/8168/8211/8411 PCI Express Gigabit Ethernet vendor: ASRock driver: r8169 v: kernel port: f000 bus-ID: 05:00.0 IF: enp5s0 state: up speed: 1000 Mbps duplex: full mac: Drives: Local Storage: total: 465.76 GiB used: 26.92 GiB (5.8%) ID-1: /dev/nvme0n1 vendor: Kingston model: SA2000M8500G size: 465.76 GiB temp: 36.9 C Partition: ID-1: / size: 453.86 GiB used: 26.65 GiB (5.9%) fs: ext4 dev: /dev/nvme0n1p3 Swap: ID-1: swap-1 type: partition size: 8 GiB used: 0 KiB (0.0%) dev: /dev/nvme0n1p2 Sensors: [...] Info: Memory: total: 16 GiB available: 15.55 GiB used: 4.37 GiB (28.1%) Processes: 294 Uptime: 1h 10m Init: systemd Packages: 1128 Compilers: clang: 17.0.6 gcc: 13.2.1 Shell: fish v: 3.7.1 inxi: 3.3.33 ```

kitty --version

```raw kitty 0.33.1 created by Kovid Goyal ```

diegodario88 commented 3 months ago

Hey there,

It looks like we might be dealing with a "kitty" (wayland) issue here. I'm not entirely sure if your patch will solve it without causing any other problems.

During my investigations, I found that listening to the clutter actor event stage-views-changed seems like a promising alternative to size-changed for meta windows. However, keep in mind that if kitty isn't emitting size changes, other behaviors might still be affected.

Have you had a chance to test this with the latest version of the kitty master branch? That might shed some light on whether the issue persists.

Thanks!

TiagodePAlves commented 3 months ago

Haven't tried the master branch yet. Will check it out later today. This stage-views-changed event sounds interesting, I will also be trying it out later.

TiagodePAlves commented 3 months ago

So, I tried the master branch today, but the problem is still there. I added some logs with terminalWindow.get_frame_rect() throughout the code and realized that kitty window already had the correct size and placement before calling _fitTerminalToMainMonitor(). Then I remembered that this was caused by the remember_window_size config that comes enabled by default. Turning it off started triggering size-changed again.

`Window::size-changed`

It seems that size-changed is not always emitted after a move_resize_frame(), and will never trigger if the terminal had the correct size beforehand. This feels like an unreliable way to check if the terminal is ready to be shown.

`Actor::stage-views-changed`

This event is emitted correctly after move_resize_frame() (update: not really true), but it is triggers a bit too early. The logs showed that the terminal was not yet resized here.

However, this event seems like a good one to check if the window frame is correct. If the size is correct, then size-changed was already emitted or it may never be emitted, so it this may be a good moment to remove the clip. If the size is wrong, then we can wait for a size-changed and let it remove the clip.

New proposed solution (NOT WORKING)

   _adjustTerminalWindowPosition() {
    /* ... */
    const mapSignalHandler = (sig, wm, metaWindowActor) => {
        /* ... */
        const sizeChangedSignalConnector = Util.once(
            this.terminalWindow,
            "size-changed",
            () => {
                if (this._internalState !== Util.TERMINAL_STATE.RUNNING) {
                    this._internalState = Util.TERMINAL_STATE.RUNNING;
                    this.actor.remove_clip();
                    this._showTerminalWithAnimationTopDown();
                }
            },
        );
        this._connectedSignals.push(sizeChangedSignalConnector);

        const stageViewsChangedSignalConnector = Util.once(
            this.actor,
            "stage-views-changed",
            () => {
                if (this._internalState !== Util.TERMINAL_STATE.RUNNING && this._terminalFrameIsCorrect()) {
                    this._internalState = Util.TERMINAL_STATE.RUNNING;
                    this.actor.remove_clip();
                    this._showTerminalWithAnimationTopDown();
                }
            },
        );
        this._connectedSignals.push(stageViewsChangedSignalConnector);

        this._fitTerminalToMainMonitor();
    }
    /* ... */
   }

I will update the PR with this proposal to better illustrate the idea.

TiagodePAlves commented 3 months ago

Nevermind, stage-views-changed is also not reliable with kitty's remember_window_size. Tomorrow or some other day I'll try to set the initial window size for other terminals like Alacritty and see if this problem is kitty-only or not.

If this indeed happens only with kitty, then maybe recommending to disable remember_window_size should be good enough.

diegodario88 commented 3 months ago

Ok, thanks a lot! I really appreciate you diving into this and being so open to trying different solutions.

TiagodePAlves commented 3 months ago

Updates from my new experiments today. First off, this problem is not restricted to kitty, I could also trigger it on alacritty and kgx (GNOME Console). Secondly, stage-views-changed is actually reliable, I must have set the listener wrong in some of my tests yesterday. Today I tried some new events to help me understand what is going on.

Terminal configurations

kitty
- remember_window_size true
- remember_window_size false, but initial_window_width and initial_window_height set (i.e. fixed size)
- remember_window_size false and no initial size
alacritty
- no configuration
- window.dimensions set (i.e. fixed size)
- window.dimensions and window.position set (i.e. fixed size and position)
kgx
- no configuration

`Window::shown`

This signal seems to always be emitted, but it is the first one to do so. It also shows up too early, and the window has not resized yet.

`Actor::stage-views-changed`

This event is also always triggered and emitted too early, but after Window::shown. It seems to be emitted right before the window is getting ready, so right before a Window::size-changed, when that happens.

Even if the window is not completely ready, this signal might be a good place to remove the clip and start animating. I tried all terminal configurations with this option and couldn't see any graphical artifact.

`Window::position-changed`

This signal is usually triggered right before a Window::size-changed, but it does happen sometimes without any size-changed. Apparently, the initial position for windows isn't really respected by GNOME, even when forced in alacritty.

With that said, this event is also not reliable. It wouldn't trigger sometimes, even with kgx. In fact, I got a hard crash when it didn't trigger, because SignalConnector was trying to disconnect from a destroyed object.

Stack trace

*Line numbers for `quake-mode.js` might not match the current master branch.* ```log abr 16 01:12:39 marmis-arch gnome-shell[1145]: Object .MetaWindowActorWayland (0x61196b849dc0), has been already disposed — impossible to access it. This might be caused by the object having been destroyed from C code using something such as destroy(), dispose(), or remove() vfuncs. == Stack trace for context 0x611968a21ef0 == #0 7ffd8c847380 b resource:///org/gnome/gjs/modules/core/overrides/GObject.js:759 (17677059d470 @ 159) #1 7ffd8c847b00 b file:///home/marmis/.local/share/gnome-shell/extensions/quake-terminal@diegodario88.github.io/util.js:46 (15ce7ab15330 @ 61) #2 611968b239a0 i file:///home/marmis/.local/share/gnome-shell/extensions/quake-terminal@diegodario88.github.io/quake-mode.js:160 (15ce7ab15920 @ 10) #3 7ffd8c848770 b self-hosted:203 (176770597ab0 @ 245) #4 611968b23918 i file:///home/marmis/.local/share/gnome-shell/extensions/quake-terminal@diegodario88.github.io/quake-mode.js:160 (15ce7ab15880 @ 367) #5 611968b23898 i file:///home/marmis/.local/share/gnome-shell/extensions/quake-terminal@diegodario88.github.io/quake-mode.js:246 (15ce7ab15ab0 @ 12) #6 611968b23808 i resource:///org/gnome/shell/ui/init.js:21 (176770570bf0 @ 48) ```

`Window::size-changed`

The current used signal and also not reliable. The event will never be emitted if the window frame already has the correct size (so that move_resize_frame does nothing), but it might also not be emitted in other situations (could figure out when). This event could be skipped with all terminals, including kgx.

But even when this is emitted, the terminal might not have reached the correct size yet (get_frame_rect() returning a different value from the expected), which indicates that starting the animation before the resizing is complete should be okay.

`Actor::show`

This is triggered way after everything else, up to 20 seconds after the _adjustTerminalWindowPosition is called. I don't think it is useful for the extension.

Conclusion

Maybe using stage-views-changed, as you proposed early on, might be the best solution here. I couldn't test on X11, but starting the animation on this event seems to work reliably well on Wayland, on all terminal configurations tested.

diegodario88 commented 3 months ago

Hey there! Just wanted to give you a quick update—I ran some tests, and everything looks good to go! I'm thinking we can merge this and give it a whirl for a few days before submitting it to GNOME extensions.

Also, it would be awesome if we could include a list of terminals that are currently compatible. I know I can't ask you to do it, but you've done an amazing job already. Thanks again for all your hard work!

TiagodePAlves commented 3 months ago

No problem! Happy to help.

I could try and formalize a better report for these terminals later on. Another option is running some automated tests like GNOME Shell does or maybe using podman. I will report back if I have any success.

But yeah, PR #33 is ready to be merged, I believe. Thanks for your help on finding a good solution for this.

diegodario88 / quake-terminal