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

Conflicting keybindings are not working in gnome 43 #533

Closed
Lythenas opened this issue May 28, 2023 · 17 comments · Fixed by #539 or #540
Closed

Conflicting keybindings are not working in gnome 43 #533

Lythenas opened this issue May 28, 2023 · 17 comments · Fixed by #539 or #540
Labels
bug Undesirable behavior

Comments

@Lythenas
Copy link
Collaborator

Describe the bug
Conflicting keybindings (e.g. Super+Shift+Left/Right to navigate monitors) are not working if they are also bound in gnome itself.

To Reproduce
Steps to reproduce the behavior:

  1. Press a conflicting keybinding (e.g. Super+Shift+Right)
  2. Nothing happens

Note that not even the action it is bound to in gnome is executed.

Removing the conflicting keybinding in gnome fixes this issue.

Expected behavior
PaperWM keybindings should take precedence.

Screenshots
If applicable, add screenshots to help explain your problem.

System information:
Please execute ./gather-system-info.sh in you PaperWM clone and paste the output below.

Distribution: Arch Linux
GNOME Shell 43.5
Display server: Wayland
PaperWM branch/tag: restore-faster
PaperWM commit: 0b73c0b1b0a41b6113fdf9f3d85ca8269a7c2157
Enabled extensions:
- paperwm@hedning:matrix.org
- [email protected]
- [email protected]

Additional context
Note that the commit is from PR #532 which is one commit ahead of develop.

@jtaala you recently made changes to fix the issue of conflicting keybindings on Gnome 44. Could this have broken the behavior on Gnome 43?

Also note that I am now updating to Gnome 44 and now have only a machine with Gnome 44 and Gnome 42 to test. But I wanted to put this up. In case it is easy to do, I think we should fix this. Otherwise, I think the gnome43 branch works fine.

@Lythenas Lythenas added the bug Undesirable behavior label May 28, 2023
@jtaala
Copy link
Collaborator

jtaala commented May 28, 2023

Ha, that is strange. I can confirm that those keybinds work fine on gnome 44 (with/without the gnome system clashes).

Could you test something for me? If you're on Gnome 43.5, could you confirm the issue doesn't occur of gnome-43 branch. If it does, please checkout v43.1.1 tag, logout/login, and see if this issue is still there?

We backported the keybind changes from Gnome 44 to Gnome 43 branch, so if the above tests works okay, then it must be something else in Gnome 44 branch (develop) that caused this.

In any case, I don't have Gnome 43 to test (and hence won't be able to reproduce/fix gnome 43 issues any longer), and there's no guarantee that future PaperWM changes in Gnome 44 won't break things in Gnome 43 (or 42, 41, or 40), hence, we've created the gnome-43 branch for users on the older Gnome 43.

@jtaala
Copy link
Collaborator

jtaala commented May 28, 2023

I'm of a mind to remove Gnome 42, 43 support in the develop branch metadata.json - we have branches for those older versions that are at a state that worked well with them. Unless we have developers that can test every new change also on Gnome 43 (and gnome 42, 41, 40,...?,...) then it's not feasible to assume that future PaperWM changes (for future Gnome versions) will always work with older Gnome versions (especially when Gnome can and does change things drastically).

@Lythenas
Copy link
Collaborator Author

That does seem reasonable if you run on e.g. Archlinux. But my guess is that distros like Ubuntu are still most commonly used (although I don't know if that is also true for PaperWM users).

If possible, I think we should continue to support the last Ubuntu LTS (which at the moment is Ubuntu 22.04, which still have Gnome 42). Also I think it is nearly impossible to install newer gnome versions on older Ubuntu distros.

But maybe I'm a bit biased because I have to use Ubuntu 22.04 at work^^ And unfortunately there also won't be an update to Ubuntu 23, since it is not LTS.

But I'm totally fine with removing anything older than 42. Also it may be easier to backport the changes separately to gnome 42, instead of maintaining it all on one branch.

@jtaala
Copy link
Collaborator

jtaala commented May 29, 2023

The main issue I find I have is that I don't run gnome 43, 42 (or 41, 40, 3.38, etc.).. and if I don't run them I can't test them... if I can't test them, I can't fix them... if I can't fix them then I can't really maintain them.

For example, I find I can't really do much with reports like "the current develop branch doesn't work on gnome 43 properly... or gnome 42 properly) - other than maybe "you could try this..." or "please just use the gnome-43 branch please since it used to work".

In order to support prev gnome versions - other than keeping the branches that worked on those versions - we need developers who use those versions and can test and backport fixes etc. where needed (or fix issues on still on those versions).

I'm happy to keep the "43", "42" in the metadata.json, but all I"ll be able to say is --- "please fix that in that older gnome version and submit a PR". E.g. it'll be up to others to submit PR's to fix issues in those gnome versions for develop branch.

I think a much better alternative given these issues is for users of older gnome versions to use the associated branch (e.g. gnome-42, gnome-43) and if there are issues they find (in those branches) then to fix them and submit PRs (or if they identify a fix in current develop request a backport if they're able to test it). That aligns what the README.md states.

@Lythenas
Copy link
Collaborator Author

Lythenas commented May 29, 2023

I just set up a Ubuntu 22.10 VM (with gnome 43.1). There on the gnome-43 branch with the default gnome keybindings Super+Right does not work. I reverted a commit ti fix the issue. See PR #537

Regarding how to handle gnome 43 and 42 testing. I would am willing to test changes for gnome 43 in a VM (like I did for this). But since I also no longer actively use gnome 43, I probably won't create backports or fixes myself. Since I still use gnome 42 actively I might then create backports/fixes for the gnome-42 branch.

I've come around on the idea of setting the develop branch to just support gnome 44 (in the metadata.json). I think that is probably overall the easiest. And I guess it is most clear way to communicate to the user that the combination of gnome and paperwm is not supported, after they update (either gnome or paperwm).

@lediur
Copy link
Contributor

lediur commented May 29, 2023

Confirm that @Lythenas's PR also fixes a similar keybinding bug that was tripping me up the past few weeks.

gather-system-info.sh (working)

Please include this information in your bug report on GitHub!
Distribution: Debian GNU/Linux
GNOME Shell 43.4
Display server: Wayland
PaperWM branch/tag: fix-keybindings-g34
PaperWM commit: c07add165b586998d1972d08964c9a45fc03c2f8

On develop or the current gnome-43 branch at https://github.com/paperwm/PaperWM/tree/9a1611a23c30386f1b6b7ce148470d96f47b730c, the <Super>1-9 keybindings for switching or launching applications from the Dock would not work with PaperWM enabled as the only extension. After turning off the extension, attempting to use the keybindings would crash the GNOME session.

With the PR, the keybindings work as expected 🥳

@jtaala
Copy link
Collaborator

jtaala commented May 29, 2023

Found it. So, interestingly, it's this line that breaks keybinds in Gnome 43 & Gnome 42 (basically prior to Gnome 44) and is only need on Gnome 44:

let [ok, key, mask] = Gtk.accelerator_parse(keystr);

In Gnome 44 the return was changed to 3 arguments. I just tested in Gnome 43 (did the same with a VM).

This means that we just need to revert keybind backports from Gnome 44 in Gnome 43 etc.

@jtaala
Copy link
Collaborator

jtaala commented May 29, 2023

Or in other words, just revert that line (removing the 'ok').

@jtaala
Copy link
Collaborator

jtaala commented May 29, 2023

Hey all, big thanks to @Lythenas for testing this out and creating some PRs.

I've now implemented a multi-gnome version fix/PR that should fix this once and for all - see #539 for details.

@lediur, @Lythenas can you please test this PR/branch. I've testing on Gnome 43 (just bit the bullet and installed a 43 vm) and also Gnome 44.

git fetch --all
git checkout fix-regression-gnome43-keybindclash
./install.sh

and then logout/login.

Thanks

@jtaala
Copy link
Collaborator

jtaala commented May 29, 2023

@Lythenas - could you also test this PR on Gnome 42? This should fix this original PR issue on 42 using same code as Gnome 44 too.

Scrap that - after merging this I'll port #539 change to develop branch and get you to test that (once done) on Gnome 42.

@lediur
Copy link
Contributor

lediur commented May 29, 2023

Just tried out the branch. Keyboard shortcuts appear to work as expected. Thanks for your work on this fix!

jtaala added a commit that referenced this issue May 30, 2023
… earlier) keybind clash detection (#539)

See #537 #538 PR's which reverts changes that were backported to Gnome
43/42. This PR is a replacement for these individual reverts PRs.

This PR implements a fix which first checks the number elements returned
from Gtk.accelerator_parse (whether 2 or 3) and assigns them correctly.
This is a much better/robust fix that means that this should now work
regardless of what gnome version is used.

Fixes #533, #536.

The issue was caused by Gtk.accelerator_parse result changing in Gnome
44. There is confusion about when this change occurred, e.g. [GJS
porting
guide](https://gjs.guide/extensions/upgrading/gnome-shell-40.html#gtk-accelerator-parse)
suggests this change occurred in Gnome 40, however actually testing in
Fedora 37 (and purportedly in other Gnome 43, 42) versions it's still
returning 2 elements instead of 3.

This PR addresses both cases.
@jtaala
Copy link
Collaborator

jtaala commented May 30, 2023

Hey all,

This should now be fixed in gnome-43 branch. I'm about to do up a PR implemented this fix in the develop branch.

@jtaala
Copy link
Collaborator

jtaala commented May 30, 2023

I've come around on the idea of setting the develop branch to just support gnome 44

Interesting! I'm coming around also to the idea that there might be a chance (and we should at least try a bit more) to have develop working in Gnome 43 & Gnome 42... the issue will be testing though - and having a wide range of testers on the different versions.

What about this?:

  • leave the older version in metadata.json in develop branch... allow (even encourage??) previous gnome versions to try develop;
  • if that fails for them (or they find bugs) - they could always use the gnome-43/42 branches until we can get to fixes in and tested in prev gnome version??

Anyways, just thinking out loud.

Btw, I've created #540, which aims to help this a bit (at least with the keybinds which should work on 44 and previous versions). Aiming to merge that in develop as soon as can.

@Lythenas
Copy link
Collaborator Author

I would be ok with either approach. But we should clearly communicate which versions are supported.

jtaala added a commit that referenced this issue May 31, 2023
#540)

This PR relates to #539.

#539 implemented a previous-gnome version backwards compatible change
for keybind clash detection.

Closes #533.

Gnome 44 changed `Gtk.accelerator_parse` result, that originally broke
keybind clash detection. The result of `Gtk.accelerator_parse` differs
in Gnome 44 and previous gnome versions. To make matters worse, there is
some confusion about when this change occurred, e.g. [GJS porting
guide](https://gjs.guide/extensions/upgrading/gnome-shell-40.html#gtk-accelerator-parse)
suggests this change occurred in Gnome 40, however actually testing in
Fedora 37 (and purportedly in other Gnome 43, 42) versions, this change
wasn't Gnome 43, 42, 41, or 40... but is in Gnome 44.

In any case, this change implements a fix that will work in all gnome
versions (or more specifically, will work correclty with the different
`Gtk.accelerator_parse` results. In essence, it checks the output and
correctly assigns the result elements (regardless of gnome version).

This PR is largely aimed at users that want to use (and test) the
current `develop` branch with older gnome versions. Ideally, we would
like to have `develop` branch compatible with previous gnome versions.
This PR at least fixes one issue and brings us closer to that ideal.
@jtaala
Copy link
Collaborator

jtaala commented Jun 2, 2023

I would be ok with either approach. But we should clearly communicate which versions are supported.

Okay, if you've been using Gnome 42 fine on develop, then I'm happy to update README.md to state gnome versions 42-43 should use develop branch. Will put a note re the version branch still being available if you run into issues (or while waiting for them to be fixed etc.).

@jtaala
Copy link
Collaborator

jtaala commented Jun 2, 2023

Will do up a PR for that today to get others' thoughts.

@jtaala
Copy link
Collaborator

jtaala commented Jun 3, 2023

See #542.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Undesirable behavior
Projects
None yet
3 participants