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

Allow KeePassXC to be built without X11 #8147

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

ya-isakov
Copy link
Contributor

KeePassXC can be built without X11 (with disabled autotype) before commit 404fd94.
Current commit is restoring this behaviour, by option WITH_XC_X11 (enabled by default)

Testing strategy

Build with -DWITH_XC_X11=no, checking that it can be built on systems with qtx11extras and x11 not installed. After that, check that it can be started and is working properly (except autotype and capslock)

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)
  • ✅ New feature (change that adds functionality)

@phoerious
Copy link
Member

phoerious commented Jun 13, 2022

Edit: I checked your changes again, they primarily patch out global hotkey handling, which isn't critical, since Auto-Type isn't supported yet anyway, but I would still prefer a Wayland implementation instead of just falling back to a noop.

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jun 13, 2022

@phoerious I'm sorry, but it seems that there is still no solution for global hotkeys in Wayland community (https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/73, #2281). Could we please merge this, for now, as it's preventing build and usage of KeepassXC on X11-less systems?

@phoerious
Copy link
Member

phoerious commented Jun 13, 2022

Sigh. I definitely stopped being surprised at what other basic shit is still missing in Wayland a long time ago. No wonder that even ten years later this thing still isn't gaining traction. 🙄

@droidmonkey Opinions?

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2022

Codecov Report

Merging #8147 (29e329e) into develop (09df862) will increase coverage by 0.20%.
The diff coverage is 22.22%.

❗ Current head 29e329e differs from pull request most recent head e09ab63. Consider uploading reports for the commit e09ab63 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8147      +/-   ##
===========================================
+ Coverage    64.44%   64.64%   +0.20%     
===========================================
  Files          339      338       -1     
  Lines        43749    43549     -200     
===========================================
- Hits         28191    28149      -42     
+ Misses       15558    15400     -158     
Impacted Files Coverage Δ
src/gui/osutils/nixutils/NixUtils.cpp 43.00% <22.22%> (+9.47%) ⬆️
src/gui/DatabaseOpenDialog.cpp 84.11% <0.00%> (-0.98%) ⬇️
src/cli/Show.cpp 97.30% <0.00%> (-0.26%) ⬇️
src/core/Entry.cpp 83.78% <0.00%> (-0.20%) ⬇️
src/cli/Utils.cpp 75.11% <0.00%> (-0.20%) ⬇️
src/cli/Clip.cpp 95.00% <0.00%> (-0.15%) ⬇️
src/gui/Clipboard.h 100.00% <0.00%> (ø)
src/core/EntryAttributes.cpp 81.18% <0.00%> (ø)
src/gui/osutils/nixutils/X11Funcs.cpp
src/gui/ApplicationSettingsWidget.cpp 52.01% <0.00%> (+0.58%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@droidmonkey
Copy link
Member

We won't be shipping an X11-less version, but I don't care if individuals want an easier way to build an X11-less version. I also agree that Wayland lacks basic features... and they wonder why its been such a struggle for adoption.

CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui/osutils/nixutils/NixUtils.cpp Outdated Show resolved Hide resolved
@ya-isakov
Copy link
Contributor Author

I'll update PR soon, addressing all the comments, thank you!

@droidmonkey droidmonkey added this to the v2.7.2 milestone Jul 2, 2022
@ya-isakov
Copy link
Contributor Author

I'm sorry, as this is approved, what are my next steps? Do I need to press some button?

@droidmonkey
Copy link
Member

No you are done, when I do my next run of PRs I'll merge this in.

@ya-isakov
Copy link
Contributor Author

Got it, thank you!

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.

4 participants