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

re-enable wayland #3520

Merged
merged 1 commit into from Oct 14, 2019

Conversation

@Gigadoc2
Copy link
Contributor

Gigadoc2 commented Sep 9, 2019

Type of change

(not sure what the appropriate type is)

  • New feature (non-breaking change which adds functionality)

Description and Context

This removes the unsetting of XDG_SESSION_TYPE, and thus restores QTs behaviour of choosing the platform backend according to OS and desktop used.

Rationale:
For one, the wayland backend of QT is quite stable nowadays, and with current QT, KPXC works fine under GNOME and sway (and probably KDE as well), file open dialog and clipboard included. Of course, auto-type does not work as with X, but that is an intentional limitation of wayland, and using KPXC under XWayland will not change this.
But even if there are distributions or desktops where QT-wayland does not work well, this problem will affect not only KPXC, but any other QT app as well (and it is likely that the user is using at least one other QT application). Thus, the user will have to override QTs backend anyway, to make all their other applications work. On the other hand, there is a big downside to setting QT_QPA_PLATFORM explicitly (which KPXC requires without this change): Forcing QT to one specific backend will not work when regularly switching between wayland and X11 desktops. But when QT_QPA_PLATFORM is unset, QT will choose the proper backend according to the desktop in use.

Testing strategy

Tested this change with GNOME Shell 3.32.2 and sway version 1.2, everything seems to be working (including file open dialogs and clipboard).

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • ☐ All new and existing tests passed. [REQUIRED]
  • ☐ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Two tests fail because of memory leaks detected, but the tests also fail without my change.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 9, 2019

I'm ok with this as long as we show that Auto-Type is disabled due to incompatibility with Wayland at this time

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 9, 2019

This can be a message on the Auto-Type settings

@Gigadoc2

This comment has been minimized.

Copy link
Contributor Author

Gigadoc2 commented Sep 9, 2019

That is a good idea, though I'd say that it is technically unrelated to this change, as Auto-Type is not possible with XWayland either (well, it might be possible for other XWayland windows, but for a user it is not obvious which applications are native wayland and which are Xwayland).

But in any case, I'll look at how to do this :)

I also realized that I might want to run more tests: Apart from not having tested KDE as a prominent Wayland desktop, I did have QT_QPA_PLATFORMTHEME=qt5ct set, which while not affection functionality might very well hide graphical inconsistencies.

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Sep 9, 2019

#2635 introduced this change

@Gigadoc2

This comment has been minimized.

Copy link
Contributor Author

Gigadoc2 commented Sep 25, 2019

I am currently testing the behaviour under GNOME with a fresh Fedora 30 install. The theming issues have become less severe (you can nicely compare a F30 with and without updates), but the mouse cursor size is still off (of course, the window decoration is also different, but that is a deliberate design descision by GNOME).

However, it turns out that exactly this workaround has become part of QT meanwhile, just limited to only force X11 under GNOME (and not other desktops).

This is the more sensible approach (though IMO the most sensible approach would be to just not disable anything and let GNOME sort out it's theme handling), so I feel like this further deprecates the workaround included here in KPXC. The version present in QT also does not prevent the usage on wayland on other Desktops (which are without theming issues).

I would argue that this can now be merged, even without the warning in the Auto-Type settings, as Auto-Type is also broken as it is now: When forcing KPXC to run with the X11 backend under wayland, Auto-Type will only work for other XWayland windows (and this might even be compositor-specific). I'd argue this is almost worse than not working at all, because you cannot see which window is native wayland and which is XWayland without digging deep into the details of your desktop, thus the Auto-Type feature will seem completely unreliable to the normal user.

@Gigadoc2

This comment has been minimized.

Copy link
Contributor Author

Gigadoc2 commented Sep 25, 2019

I just tested with KDE as well, there the theming works better (sort of expected with KPXC being a QT app), just the favicon is missing, which is an interesting bug.

BTW, I just noticed that the entire Auto-Type tab is missing anyway when native wayland is in use. Maybe this behaviour should be extended to XWayland as well (instead of the warning message)?

@droidmonkey droidmonkey requested a review from phoerious Sep 28, 2019
@droidmonkey droidmonkey merged commit 82cfedf into keepassxreboot:develop Oct 14, 2019
4 checks passed
4 checks passed
CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details
@droidmonkey droidmonkey added this to the v2.5.0 milestone Oct 16, 2019
phoerious added a commit that referenced this pull request Oct 26, 2019
Added

- Add 'Paper Backup' aka 'Export to HTML file' to the 'Database' menu [[#3277](#3277)]
- Add statistics panel with information about the database (number of entries, number of unique passwords, etc.) to the Database Settings dialog [[#2034](#2034)]
- Add offline user manual accessible via the 'Help' menu [[#3274](#3274)]
- Add support for importing 1Password OpVault files [[#2292](#2292)]
- Implement Freedesktop.org secret storage DBus protocol so that KeePassXC can be used as a vault service by libsecret [[#2726](#2726)]
- Add support for OnlyKey as an alternative to YubiKeys (requires yubikey-personalization >= 1.20.0) [[#3352](#3352)]
- Add group sorting feature [[#3282](#3282)]
- Add feature to download favicons for all entries at once [[#3169](#3169)]
- Add word case option to passphrase generator [[#3172](#3172)]
- Add support for RFC6238-compliant TOTP hashes [[#2972](#2972)]
- Add UNIX man page for main program [[#3665](#3665)]
- Add 'Monospaced font' option to the notes field [[#3321](#3321)]
- Add support for key files in auto open [[#3504](#3504)]
- Add search field for filtering entries in Auto-Type dialog [[#2955](#2955)]
- Complete usernames based on known usernames from other entries [[#3300](#3300)]
- Parse hyperlinks in the notes field of the entry preview pane [[#3596](#3596)]
- Allow abbreviation of field names in entry search [[#3440](#3440)]
- Allow setting group icons recursively [[#3273](#3273)]
- Add copy context menu for username and password in Auto-Type dialog [[#3038](#3038)]
- Drop to background after copying a password to the clipboard [[#3253](#3253)]
- Add 'Lock databases' entry to tray icon menu [[#2896](#2896)]
- Add option to minimize window after unlocking [[#3439](#3439)]
- Add option to minimize window after opening a URL [[#3302](#3302)]
- Request accessibility permissions for Auto-Type on macOS [[#3624](#3624)]
- Browser: Add initial support for multiple URLs [[#3558](#3558)]
- Browser: Add entry-specific browser integration settings [[#3444](#3444)]
- CLI: Add offline HIBP checker (requires a downloaded HIBP dump) [[#2707](#2707)]
- CLI: Add 'flatten' option to the 'ls' command [[#3276](#3276)]
- CLI: Add password generation options to `Add` and `Edit` commands [[#3275](#3275)]
- CLI: Add XML import [[#3572](#3572)]
- CLI: Add CSV export to the 'export' command [[#3278](#3278)]
- CLI: Add `-y --yubikey` option for YubiKey [[#3416](#3416)]
- CLI: Add `--dry-run` option for merging databases [[#3254](#3254)]
- CLI: Add group commands (mv, mkdir and rmdir) [[#3313](#3313)].
- CLI: Add interactive shell mode command `open` [[#3224](#3224)]

Changed

- Redesign database unlock dialog [ [#3287](#3287)]
- Rework the entry preview panel [ [#3306](#3306)]
- Move notes to General tab on Group Preview Panel [[#3336](#3336)]
- Enable entry actions when editing an entry and cleanup entry context menu  [[#3641](#3641)]
- Improve detection of external database changes  [[#2389](#2389)]
- Warn if user is trying to use a KDBX file as a key file [[#3625](#3625)]
- Add option to disable KeePassHTTP settings migrations prompt [[#3349](#3349), [#3344](#3344)]
- Re-enabled Wayland support (no Auto-Type yet) [[#3520](#3520), [#3341](#3341)]
- Add icon to 'Toggle Window' action in tray icon menu [[3244](#3244)]
- Merge custom data between databases only when necessary [[#3475](#3475)]
- Improve various file-handling related issues when picking files using the system's file dialog [[#3473](#3473)]
- Add 'New Entry' context menu when no entries are selected [[#3671](#3671)]
- Reduce default Argon2 settings from 128 MiB and one thread per CPU core to 64 MiB and two threads to account for lower-spec mobile hardware [ [#3672](#3672)]
- Browser: Remove unused 'Remember' checkbox for HTTP Basic Auth [[#3371](#3371)]
- Browser: Show database name when pairing with a new browser [[#3638](#3638)]
- Browser: Show URL in allow access dialog [[#3639](#3639)]
- CLI: The password length option `-l` for the CLI commands `Add` and `Edit` is now `-L` [[#3275](#3275)]
- CLI: The `-u` shorthand for the `--upper` password generation option has been renamed to `-U` [[#3275](#3275)]
- CLI: Rename command `extract` to `export`. [[#3277](#3277)]

Fixed

- Improve accessibility for assistive technologies [[#3409](#3409)]
- Correctly unlock all databases if `--pw-stdin` is provided [[#2916](#2916)]
- Fix password generator issues with special characters [[#3303](#3303)]
- Fix KeePassXC interrupting shutdown procedure [[#3666](#3666)]
- Fix password visibility toggle button state on unlock dialog [[#3312](#3312)]
- Fix potential data loss if database is reloaded while user is editing an entry [[#3656](#3656)]
- Fix hard-coded background color in search help popup [[#3001](#3001)]
- Fix font choice for password preview [[#3425](#3425)]
- Fix handling of read-only files when autosave is enabled [[#3408](#3408)]
- Handle symlinks correctly when atomic saves are disabled [[#3463](#3463)]
- Enable HighDPI icon scaling on Linux [[#3332](#3332)]
- Make Auto-Type on macOS more robust and remove old Carbon API calls [[#3634](#3634), [[#3347](#3347))]
- Hide Share tab if KeePassXC is compiled without KeeShare support and other minor KeeShare improvements [[#3654](#3654), [[#3291](#3291), [#3029](#3029), [#3031](#3031), [#3236](#3236)]
- Correctly bring window to the front when clicking tray icon on macOS [[#3576](#3576)]
- Correct application shortcut created by MSI Installer on Windows [[#3296](#3296)]
- Fix crash when removing custom data [[#3508](#3508)]
- Fix placeholder resolution in URLs [[#3281](#3281)]
- Fix various inconsistencies and platform-dependent compilation bugs [[#3664](#3664), [#3662](#3662), [#3660](#3660), [#3655](#3655), [#3649](#3649), [#3417](#3417), [#3357](#3357), [#3319](#3319), [#3318](#3318), [#3304](#3304)]
- Browser: Fix potential leaking of entries through the browser integration API if multiple databases are opened [[#3480](#3480)]
- Browser: Fix password entropy calculation [[#3107](#3107)]
- Browser: Fix Windows registry settings for portable installation [[#3603](#3603)]
@GabrielPrototype

This comment has been minimized.

Copy link

GabrielPrototype commented Nov 16, 2019

It looks like AutoType in wayland is possible via a virtual keyboard device, someone has already implemented a prototype for KeepassX, please take a look at the code he wrote https://github.com/rockihack/keepassx/tree/wayland-autotype/src/autotype/wayland

cc: @rockihack

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 16, 2019

Wow nice!

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Nov 16, 2019

Yeah, I thought about something like that. There are onscreen keyboards for Wayland after all. The standardisation and implementation of protocol extensions to enable assistive technologies are still lacking a lot, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.