Skip to content

Conversation

friday
Copy link
Member

@friday friday commented Aug 13, 2023

This replaces the keybinder dependency with a custom DE-wrapper to set the hotkey via DE-specific commands. Currently supports Gnome, KDE and XFC, but only the Gnome one has been tested.

The benefit is that this will work on both X11 and Wayland, but it won't work on unsupported DEs until users set it up themselves. There are system notifications to guide you though this though.

Rewrote much of the old code for setting the hotkey with a new better looking and more standardized dialog, and tried to make the notifications clearer (we need that now).

First launch with this PR it will try to migrate your setting or add the default, then show this

If your DE is supported it will migrate the setting to the DE shortcut settings, and show this

Screenshot from 2023-08-13 18-43-28
If it's the first time you open Ulauncher, it will default to Ctrl+space (like before)

If your DE is not supported, it will show this instead

Screenshot from 2023-08-13 18-27-32
And as it says, you will open the preferences by clicking it, with more details:
image

When the DE is supported, the settings look like this

Screenshot from 2023-08-13 16-28-04
Screenshot from 2023-08-13 16-27-57

Screenshot from 2023-08-13 16-28-43

Some of the details might need polish, but that can be changed later. (I have now pushed a few commits to improve the usability and robustness)

  • I don't know why the modal has so much padding. I guess it probably has an area reserved for the message label which I'm not using at all. I think that's unimportant though.

@friday friday force-pushed the feat/remove-keybinder branch from c7449e6 to 702091c Compare August 13, 2023 17:04
@friday friday added the v6 label Aug 13, 2023
@friday friday added this to the 6.0.0 milestone Aug 13, 2023
@friday friday removed the v6 label Aug 13, 2023
@friday
Copy link
Member Author

friday commented Aug 13, 2023

I'm going to merge this immediately. I just added you as reviewers @troycurtisjr @gornostal to let you know. This wasn't possible to do before because of the (now fixed) focus issue.

@friday friday merged commit 40c7c96 into v6 Aug 13, 2023
@friday friday deleted the feat/remove-keybinder branch August 13, 2023 17:08
@friday
Copy link
Member Author

friday commented Aug 13, 2023

Posted from KDE live CD (testing):

Update:

The current KDE implementation (now deactivated) didn't work for me, but this did work 100% on both X11 and Wayland without focus issues on Kubuntu with systemd:

  1. systemctl --user is-active plasma-kglobalaccel (should output "active". Then we know the service is supported)
  2. kreadconfig5 --file kglobalshortcutsrc --group io.ulauncher.Ulauncher.desktop --key "_launch" to check if it has already been set (then leave it).
  3. kwriteconfig5 --file kglobalshortcutsrc --group io.ulauncher.Ulauncher.desktop --key "_k_friendly_name" "Ulauncher" && kwriteconfig5 --file kglobalshortcutsrc --group io.ulauncher.Ulauncher.desktop --key "_launch" "Ctrl+Space,none,Ulauncher" (to set)
  4. systemctl --user restart plasma-kglobalaccel to apply the change.

This also adds it with the rest of your application shortcuts and adds an icon.
Screenshot_20230813_233453

I think that's as good as it gets. To add it as a "default shortcut" instead of "custom" I think you need to make it a native KDE app and use this SDK (example).

You can then open this view directly by running systemsettings5 kcm_keys instead of opening our custom hotkey dialog.

An older thread suggested a more naive way to restart the service, which could be coupled with pidof kglobalaccel5 to feature detect. However given how many issues we had with subprocesses getting killed when ulauncher is restarted I think systemd is the only way.

The hotkey in in another format. It looks just like the "label" format you can get from the gtk accelerator one: Gtk.accelerator_get_label(*Gtk.accelerator_parse(hotkey)), but it's not 100% the same (this would cover most of it though: replace("Super", "Meta").replace("Escape", "Esc").replace("Page Up", "PgUp").replace("Page Down", "PgDn") for example (see more here). I don't feel great about converting without knowing for sure that this will work for every possible scenario and not break the system. However, maybe we can just set it to Ctrl+Space and run systemsettings5 kcm_keys so the user can set it to what they actually want.

Old post

Did some testing on KDE, and found that this didn't actually work (based on an outdated article with fake date saying June this year).
Also, focus stealing by default is enabled (level 1 - low) and you have to disable it.
Screenshot_20230813_202445

It can be disabled programmatically with
kwriteconfig5 --file ~/.config/kwinrc --group Windows --key FocusStealingPreventionLevel 0. We shouldn't do that of course, but one quickfix would be to disable it before launching ulauncher every time, then restoring the previous setting (which can be read with kreadconfig5 --file ~/.config/kwinrc --group Windows --key FocusStealingPreventionLevel).

Also, the empty space has an icon now, so I guess that's what it's supposed to have:
Screenshot_20230813_195735

Related article about KGlobalAccel says it's only X11 anyway: https://blog.martin-graesslin.com/blog/2015/06/global-shortcut-handling-in-a-plasma-wayland-session/ Need to test this again with kubuntu wayland sudo apt install plasma-workspace-wayland (and log out/in again and choose Wayland session)

@gornostal
Copy link
Member

Great stuff @friday!
And thanks for the very detailed PR description, btw.

@friday
Copy link
Member Author

friday commented Aug 15, 2023

Have tested and fixed the KDE and XFCE implementations now, and they work great (after pushing fixes). So we have great support coverage already. I am pretty sure Budgie, Panteon and Cinnamon uses the same way as Gnome, but with different gsettings paths, so maybe I will add those or some of them. I won't add support for more than those though. Should be easy for contributors who are on smaller desktops to contribute.

@Ulauncher Ulauncher locked as resolved and limited conversation to collaborators Oct 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants