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

gtk3: disable low level keyboard hook #6244

Merged
merged 1 commit into from
Mar 4, 2020
Merged

Conversation

Ede123
Copy link
Contributor

@Ede123 Ede123 commented Feb 29, 2020

The hook is used to intercept keyboard combos so gtk is able to trigger the AeroSnap-like functionality it implements as a replacement for real AeroSnap when client-side decorations (CSD) are used on a window.

However the keyboard hook causes significant performance issues, especially on startup of gtk3 apps (those are system-wide and not limited to the gtk3 app itself). Input performance can be continuously degraded for any console application that is attached to the gtk3 app, e.g. debuggers.

Loss of the keyboard combos in CSD windows seems to be acceptable considering the severe functional limitations the hook itself causes.

This is especially true as we do not use CSD by default so we do not even depend on the (incomplete) AeroSnap-emulation GTK provides in most cases. Instead we're using native AeroSnap as provided by the OS directly, with keyboard combos working as expected.

Related issues:

@lazka
Copy link
Member

lazka commented Mar 1, 2020

Just curious: What happens to keyboard combos with CSD apps with this? They just do nothing?

@Ede123
Copy link
Contributor Author

Ede123 commented Mar 1, 2020

Yes, the code we loose is low_level_keystroke_handler().

It handles the "Windows Key+Arrow Keys" combos, but only for CSD windows, so they'll simply do nothing in the CSD case as you suspected.

@lazka
Copy link
Member

lazka commented Mar 3, 2020

What about guarding it with a if (!g_getenv ("GTK_NO_KEYBOARD_HOOK")) and setting that in your code?

@Ede123
Copy link
Contributor Author

Ede123 commented Mar 3, 2020

To be honest I don't like the idea of introducing an undocumented environment variable that is not part of the upstream API. Most people won't know about it and will continue to suffer from bad performance.

That is especially true if client side decorations are disabled (our default) because we don't have any need for the hook in this case. It certainly feels wrong to install it in the first place then, as it only has disadvantages.

I could settle for checking if GTK_CSD=1 (then install the hook), but keep it disabled otherwise. This should ensure functionality for apps that explicitly want to use CSD, but would still avoid performance degradations for all apps that don't. Would that be a reasonable compromise?

(Arguably the loss of combos might be preferable over bad performance even for the CSD case, but yes, this probably comes down to personal preference)

@lazka
Copy link
Member

lazka commented Mar 4, 2020

To be honest I don't like the idea of introducing an undocumented environment variable that is not part of the upstream API. Most people won't know about it and will continue to suffer from bad performance.

You're introducing a feature change that is not part of upstream, so I don't think it is worse, but ymmv.

That is especially true if client side decorations are disabled (our default) because we don't have any need for the hook in this case. It certainly feels wrong to install it in the first place then, as it only has disadvantages.

The reason I'm wary is that I use key hooks in my programs anyway, so gtk adding another one doesn't bother me.

I could settle for checking if GTK_CSD=1 (then install the hook), but keep it disabled otherwise. This should ensure functionality for apps that explicitly want to use CSD, but would still avoid performance degradations for all apps that don't. Would that be a reasonable compromise?

apps can use CSD without the env var, but this is better I guess, so 👍

(Arguably the loss of combos might be preferable over bad performance even for the CSD case, but yes, this probably comes down to personal preference)

The hook is used to intercept keyboard combos in order to provide
AeroSnap-like functionality for CSD windows.

However it causes significant performance issues, especially on
startup of gtk3 apps (those are system-wide and *not* limited to the
gtk3 app itself). Performance can be continuously degraded to any
console application that is attached to the gtk3 app, e.g. debuggers.

Related issues:
- https://gitlab.gnome.org/GNOME/gtk/issues/2015
- https://gitlab.gnome.org/GNOME/gtk/issues/1082
- https://gitlab.gnome.org/GNOME/gtk/issues/1033

As the hook is completely useless for non-CSD windows, only install
it if CSD is explicitly requested (environment variable GTK_CSD=1).
@Ede123 Ede123 force-pushed the gtk3_keyboard_hook branch from bba4207 to 723765a Compare March 4, 2020 18:21
@Ede123
Copy link
Contributor Author

Ede123 commented Mar 4, 2020

The reason I'm wary is that I use key hooks in my programs anyway, so gtk adding another one doesn't bother me.

AFAIU the problem is not the hook itself but how it's implemented, though:
Apparently it seems to be able to delay keyboard input events by blocking the gtk main thread! I don't have enough insight to know whether this could be done better, though, and how much effort it would be (maybe you do if you handle similar hooks as well? Ideally we'd obviously fix the hook at one point).

apps can use CSD without the env var, but this is better I guess, so 👍

Updated the MR with the env var check.

I'm aware windows can still use CSD if GTK_CSD!=1 (is there any direct way to enable it programatically short of putting widgets into the titlebar btw.?). However we should achieve reasonable behavior without any loss of functionality in the majority of cases with the updated patch, which seems to be the best we can hope for without actually fixing the underlying issues.

@lazka lazka merged commit 13297b1 into msys2:master Mar 4, 2020
@Ede123 Ede123 deleted the gtk3_keyboard_hook branch March 4, 2020 18:47
@lazka
Copy link
Member

lazka commented Mar 4, 2020

(maybe you do if you handle similar hooks as well? Ideally we'd obviously fix the hook at one point).

I have a very simple case, so likely not that easy in gtk, let's see.

is there any direct way to enable it programatically short of putting widgets into the titlebar btw.?

I think, besides having it as a default, only set_titlebar() will enable it, but not 100% sure.

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