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

Wayland autotype implementation (using xdg-desktop-portal) #10905

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

TheConfuZzledDude
Copy link

Fixes #2281

This is a work-in-progress attempt to implement auto-type on Wayland, using the RemoteDesktop portal. Caveats currently include:

  • Requires to have a desktop portal installed that supports the RemoteDesktop portal (xdg-desktop-portal-kde, xdg-desktop-portal-gnome, xdg-desktop-portal-luminous)
  • Global shortcuts aren't implemented (and trying to set one in the auto-type tab will crash), but I've added a D-Bus method which should make it fairly easy to bind a shortcut to request a global type in your DE
  • Window detection isn't implemented, as there's no cross-platform way to do this on Wayland currently, but could be done in the future for some platforms (i.e. wlr-foreign-toplevel-management, KWin scripts, etc.)
  • As a consequence of the last point, activation of foreign windows isn't possible, so typing occurring in the correct window relies on it being automatically activated when the application window is deactivated
  • A RemoteDesktop session is started when the plugin is loaded (on application launch), and the restore token isn't persisted, so it will ask every time. Ideally, a session would only be started when you request an auto-type, and would be closed when done, and the token persisted in the database, but this would need a couple changes to the how the autotype plugins work I think, so I've not touched that yet.

All testing so far I've done on KDE Plasma 6.1, and the core functionality seems to work great there. Testing other platforms is a bit annoying to do, so I'd appreciate people trying it and giving feedback

Screenshots

Testing strategy

Type of change

  • ✅ New feature (change that adds functionality)

@droidmonkey
Copy link
Member

droidmonkey commented Jun 15, 2024

Geez sure is nice when you don't have to muck with emulated key presses! I wonder if we could use this same method for X11 as well, do you know?

@TheConfuZzledDude
Copy link
Author

Geez sure is nice when you don't have to muck with emulated key presses! I wonder if we could use this same method for X11 as well, do you know?

Technically it should, as long as there's a desktop portal running that works on it, so I have a feeling KDE and GNOME would, but I don't think there's a generic portal that supports RemoteDesktop for other X11 WMs.

Also, it is very nice I can just chuck keysyms at it without worrying about reversing keymaps and whatnot :)

@droidmonkey
Copy link
Member

Worth a try to see if that fixes some of our awkward issues with international keyboard layout.

@hifi
Copy link
Member

hifi commented Jun 16, 2024

Very nice, I will give this a spin!

@hifi
Copy link
Member

hifi commented Jun 16, 2024

Applied a small patch to build on Debian bookworm and to fix some assumptions about a window title existing and the shortcut crash and to disable minimize behavior if the main window is inactive, didn't test X11 still works: https://gist.github.com/hifi/e32c03f7b131cfc9e2d38c1ca11764fe

I registered this command with KDE to perform globally: dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType

Good work! I will stay on Wayland for now as this implements everything I need. Just needs some cleanups.

@Zahrun
Copy link

Zahrun commented Jun 16, 2024

Thanks @TheConfuZzledDude for the PR
Thanks @droidmonkey for all the work on KeepassXC
Thanks @hifi for the patch

I was able to build this branch on my Tuxedo OS 3 (Plasma 6.0.5) after applying @hifi ’s patch.

If I perform AutoType from the KeePassXC window, that one does not get minimized. That is due to the && qApp->activeWindow()) added by @hifi . Works fine once reverted that line.

Assigned a global keyboard shortcut to dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType in Plasma, that’s pretty sick, just need to press the shortcut, type the name of the entry, select it, and it will autotype.

The only issue I’m having is that when I switch keyboard layout, the autotype changes. On my default fr oss layout it types correctly. Changing to fr bepo_afnor or us intl types wrong characters. It is actually pressing the same keys on the keyboard, but now they have different characters.

@hifi
Copy link
Member

hifi commented Jun 16, 2024

My bad about minimizing not working as I didn't test it properly. The intention was to not minimize the main window on global auto-type if the main window isn't the active window (aka entry auto-type).

Also it's a bit funny keyboard layouts are still an issue as they are a nightmare on X11.

@TheConfuZzledDude
Copy link
Author

Thanks @TheConfuZzledDude for the PR Thanks @droidmonkey for all the work on KeepassXC Thanks @hifi for the patch

I was able to build this branch on my Tuxedo OS 3 (Plasma 6.0.5) after applying @hifi ’s patch.

If I perform AutoType from the KeePassXC window, that one does not get minimized. That is due to the && qApp->activeWindow()) added by @hifi . Works fine once reverted that line.

Assigned a global keyboard shortcut to dbus-send --session --type=method_call --dest=org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.requestGlobalAutoType in Plasma, that’s pretty sick, just need to press the shortcut, type the name of the entry, select it, and it will autotype.

The only issue I’m having is that when I switch keyboard layout, the autotype changes. On my default fr oss layout it types correctly. Changing to fr bepo_afnor or us intl types wrong characters. It is actually pressing the same keys on the keyboard, but now they have different characters.

You know I really would hope that the desktop portal would be able to query the keymap correctly, but I guess not. I think that using libei would allow getting the actual current keymap (from reading through the KWin code, it seems like it would there at least). I have a branch locally where I started implementing it with EI, but it's sooooo much more of a hassle, and KDE doesn't actually support it until 6.1 is released (they say they implement v2 of the protocol but they don't :P), and I ran into issues where the keymap FD they were providing was perpetually invalid after the first time.

So I'd rather stick with this and maybe drop a question to the portal maintainers about whether they should be querying the current keymap when we request a keysym (or if we should do the exact same reverse mapping just with 200% extra effort)

@droidmonkey
Copy link
Member

droidmonkey commented Jun 17, 2024

We should absolutely be pushing this onto the portal maintainers to handle. Direct query of the keymap or have them align the key maps accordingly between apps. I say keep current implementation and caveat that it only applies to keymaps that are largely US layout based.

@TheConfuZzledDude
Copy link
Author

@Zahrun If you'd like a workaround for now, you can set up keyboard shortcuts in the KDE settings to switch layout to a default pc105 keyboard layout, and one to switch to the previous layout. Then, set the default auto-type sequence for your group to press the respective shortcuts

@hifi
Copy link
Member

hifi commented Jun 18, 2024

I've been using this for a couple of days now and it's been working very well in practice with the known caveats.

@TheConfuZzledDude Regarding keymap, does it make a difference if you start the remote desktop portal at the time of starting Auto-Type? If it helps then we could make it do the initial authorization for remote desktop on startup but immediately close the portal so that it can reopen it on demand to use the current keymap.

@Zahrun
Copy link

Zahrun commented Jun 18, 2024

@droidmonkey I also think that the layout management issue should be raised on the desktop portal side.

@TheConfuZzledDude thank you for the workaround suggestion. I have a lot of custom auto-type sequences, so I prefer to switch layout, then do the auto-type, and then switch back manually. I'm actually currently using a script to do that with KPUInput in KeePass2 in mono. I'll adjust it to work with your implementation in KeePassXC. I hope one day the autotype will work with any layout configuration.

@hifi In my experience, it does not matter in which layout I am when starting the portal. It seems to be always using fr oss, which is the layout I have in SDDM and in the ttys. It's not even the first layout in the layout setting list inside Plasma.

EDIT: I am able to reproduce the keyboard layout issue with KDE Connect when typing from my Android device to the Plasma desktop, through xdg-desktop-portal, the keyboard layout issue is also happening.

@Zahrun
Copy link

Zahrun commented Jun 23, 2024

I created an issue on the KDE side for xdg-desktop-portal-kde at https://bugs.kde.org/show_bug.cgi?id=489021

@TheConfuZzledDude
Copy link
Author

I created an issue on the KDE side for xdg-desktop-portal-kde at https://bugs.kde.org/show_bug.cgi?id=489021

Thanks! I was thinking of doing that myself, but I think I might post it on https://invent.kde.org/plasma/xdg-desktop-portal-kde/-/issues because it seems more active for the desktop portal

Regardless, I'm going to try and clean this up a bit in the next week

@Zahrun
Copy link

Zahrun commented Jun 26, 2024

I opened the issue on bugs.kde.org due to the following guideline.

Please note that all bug reports and feature requests should be filed on [bugs.kde.org](https://bugs.kde.org/) and should never be raised here.
For support with KDE software, it is recommended to post on [KDE Discuss](https://discuss.kde.org/) instead.

Issues on KDE Invent are used for tracking ongoing work and are for the use of contributors and developers only.

Users are asked to consult with a developer or other contributor prior to opening issues here, and where in doubt recommended to open them on bugs.kde.org instead.

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

@hifi
Copy link
Member

hifi commented Jun 26, 2024

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

It could just request the remote desktop portal when being triggered and then keep it open after that until next restart. There's no need to open the portal at all if the user never uses auto-type. That's very likely for a lot of people who mostly only use the browser integration.

@michaelk83
Copy link

@TheConfuZzledDude I think your input is needed at https://bugs.kde.org/show_bug.cgi?id=489021 :

Can you show where the bug is in our portal implementation? And maybe also update the title to describe the problem more fully?

@michaelk83
Copy link

Btw, does the desktop portal specification specify how it should deal with different keyboard mappings? If not, then this should be brought up with them as well. Otherwise, you'll have to chase each DE to get them to do what is needed.

@TheConfuZzledDude
Copy link
Author

@TheConfuZzledDude I think your input is needed at https://bugs.kde.org/show_bug.cgi?id=489021 :

Can you show where the bug is in our portal implementation? And maybe also update the title to describe the problem more fully?

Just commented, found where the issue was, depending on how fast I get through the rest of this, I may go fix it myself if it's not by the time I'm done

Btw, does the desktop portal specification specify how it should deal with different keyboard mappings? If not, then this should be brought up with them as well. Otherwise, you'll have to chase each DE to get them to do what is needed.

Not really, the remote desktop documentation is kinda sparse apart from what's implied from the method name. I did some research, and it seems like the protocol was defined for/based on Mutter's remote desktop portal, so I think that's probably the "canonical" behaviour

Apart from that, I have a suggestion for the feature. Currently, when we open KeePassXC, it directly asks for Remote Control permission. New users might get confused by that. I think it would be good to display a message "We will ask for permission, this is for autotype feature". Or even give the option yes/no. Or probably best, ask for permission only if the user asks for an autotype to be done. And then keep the token.

It could just request the remote desktop portal when being triggered and then keep it open after that until next restart. There's no need to open the portal at all if the user never uses auto-type. That's very likely for a lot of people who mostly only use the browser integration.

That is the plan, but there's a few issues to work around, and might require changing the autotype plugin behaviour in general. When you start a remote desktop session you get a restore token that can be used to restore the session without prompt in the future, and this could be retained between launches as well. The question for that then is whether you should only be prompted the first time ever, and if so, where should the restore token be stored? I guess it should be kept secure since another program could steal it and silently start controlling your inputs or something, but that means probably storing it in the database which seems kinda meh.
Also, starting the session only when the user requests an autotype has some issues too, since the way it works, as soon as they close the dialog it'll start typing, regardless of what window is now active, or how much time has passed. So I guess database unlock/auto-type being enabled would be more appropriate, but I'll need to add those to the plugin interface

@droidmonkey
Copy link
Member

droidmonkey commented Jun 27, 2024

I like the idea of one-time ask (after first database unlock maybe) and store the token in the database. A new database requires a new token. This would emulate the behavior of the browser plugin.

The downside would be if you used your database on multiple Linux wayland boxes. So you may need to add the computer name to the token storage key.

@badsmoke
Copy link

is there any progress here? can i help with testing?

@Ferk
Copy link

Ferk commented Sep 19, 2024

Window detection isn't implemented, as there's no cross-platform way to do this on Wayland currently, but could be done in the future for some platforms (i.e. wlr-foreign-toplevel-management, KWin scripts, etc.)

Would it be possible for the D-Bus method to accept as parameter a string to use for the auto-type matching?

Then for compositors that are scriptable enough (and people who can script it) it would be possible to bind a key to a script that sends the appropriate Window Title as parameter via D-Bus.

Though I think it would be important to show in the UI what is the string that is being used as parameter for security reasons, to avoid some potential form of phishing (actually.. I think that would be a good idea in general, sometimes it's annoying to not be able to tell exactly what's the title that was detected).

Alternatively (and perhaps it's a better idea) have the content of that parameter be the default value in the input for the search in the auto-type popup (so the value is visible and at the same time, editable in case a match is not found).

@Zahrun
Copy link

Zahrun commented Nov 7, 2024

@TheConfuZzledDude thank you again for the nice work in this PR
The bounty is still pending, is there any chance you could give a look at the keyboard layout situation? It is, from my side, the only pending adjustment. Actually, since it is an issue on the portal side and not in keepassxc side, we could also consider this solved.

I don’t need the window title detection for now. It is good enough if I can type to search the entry, and it gets auto-typed.

@droidmonkey what would be missing in the PR from your point of view to get this merged?

@droidmonkey droidmonkey force-pushed the autotype-wayland-xdg-portal branch 3 times, most recently from 4440fe8 to b9e0d0a Compare November 8, 2024 05:03
@droidmonkey
Copy link
Member

droidmonkey commented Nov 8, 2024

The new code written for this PR needs to be modified to meet our style. We also need to consider when someone doesn't want to build with X11 we need to separate the X11 auto-type from Wayland auto-type in the CMakeLists.

Registering the auto-type global shortcut also causes a crash because an X11 display is not valid/found. We need to guard against using X11 functions when running under wayland. We should also just hide the shortcut widget in settings when running under wayland.

Need to put Xkbcommon into the CI container

@droidmonkey droidmonkey force-pushed the autotype-wayland-xdg-portal branch 2 times, most recently from d37e8b1 to d6372c4 Compare November 9, 2024 04:15
@hifi
Copy link
Member

hifi commented Nov 13, 2024

Did some quick checks for the minimizing issue as I don't want the main window to be minimized when global auto-type is being performed which matches the X11 behavior.

Because a Wayland implementation cannot get the active window the behavior from KeePassXC is to always minimize our own if the window is unknown which should only happen with per-entry Auto-Type. The safety checks for focus loss prevention are also broken on Wayland as it's impossible to know which window we're typing to - except to our own - which is also not working.

I'm currently running with this change to the Wayland platform which emulates it enough to get the correct behavior for both per-entry Auto-Type and global without touching the Auto-Type implementation itself and it also prevents typing to the main KeePassXC window if you re-focus it by accident during the sequence:

#include <QApplication>

...

WId AutoTypePlatformWayland::activeWindow()
{
    // If we have our own window focused, return something else to stop sequence
    if (qApp->activeWindow()) {
        return 2;
    }

    // Emulate active window detection by returning non-zero window
    return 1;
}

Own window detection could be moved to the main Auto-Type executor to also check for our own focus instead of just the active window changing from the platform executor but on that side it looks redundant.

The only hack I have on top of this is the platform check on registering global shortcut to prevent the crash on the current head of this branch.

@droidmonkey droidmonkey force-pushed the autotype-wayland-xdg-portal branch 6 times, most recently from 698bf61 to 2f03e4f Compare November 26, 2024 04:11
@droidmonkey
Copy link
Member

This fails miserably with extended ascii and many symbols. I suspect that is because of the use of the X11 keymap lookups. Will have to implement lookups to XKB keysym to move this forward.

@droidmonkey droidmonkey force-pushed the autotype-wayland-xdg-portal branch from a5c6e10 to fc1bd20 Compare November 26, 2024 04:36
@droidmonkey droidmonkey marked this pull request as draft November 26, 2024 04:41
@droidmonkey droidmonkey changed the title WIP: Wayland autotype implementation (using xdg-desktop-portal) Wayland autotype implementation (using xdg-desktop-portal) Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoType not available when using Wayland
7 participants