paperwm / PaperWM

Tiled scrollable window management for Gnome Shell
GNU General Public License v3.0
3.01k stars 127 forks source link

Conflicting keybindings are not working in gnome 43 #533

Closed Lythenas closed 1 year ago

Lythenas commented 1 year ago

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
- switcher@landau.fi
- appindicatorsupport@rgcjonas.gmail.com

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.

jtaala commented 1 year ago

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

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

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

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

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

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 :partying_face:

jtaala commented 1 year ago

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:

https://github.com/paperwm/PaperWM/blob/9a1611a23c30386f1b6b7ce148470d96f47b730c/settings.js#L245

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

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

jtaala commented 1 year ago

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

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

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

jtaala commented 1 year ago

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

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?:

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

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

jtaala commented 1 year ago

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

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

jtaala commented 1 year ago

See #542.