Fix absence of border in WSL wayland environment#1829
Open
psolstice wants to merge 5 commits into
Open
Conversation
|
User peter@shugalev.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Qt Wayland patches and applies them in depends; updates static Qt Wayland plugin discovery and runtime registration; converts multiple XCB-related depends to static builds and adds libXdmcp; adds CMake cache cleanup for stale depends shared-library entries; registers XDG icon/desktop resources and sets Qt desktop file name. ChangesQt Wayland decoration & plugin changes
Depends: XCB / libXdmcp / xcb-util changes (static builds)
Build system cache hygiene
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant QWaylandDisplay as QWaylandDisplay
participant ClientIntegration as ClientBufferIntegration (EGL)
participant QWaylandWindow as QWaylandWindow
Note over App,QWaylandWindow: Environment may be SHM-only (no EGL)
App->>QWaylandDisplay: query supportsWindowDecoration()
alt QT_WAYLAND_DISABLE_WINDOWDECORATION set
QWaylandDisplay-->>App: return false
else
QWaylandDisplay->>ClientIntegration: query clientBufferIntegration() (may be absent)
Note right of QWaylandDisplay: new logic does not gate on EGL integration
QWaylandDisplay-->>App: return true
end
App->>QWaylandWindow: createDecoration()
QWaylandWindow-->>App: decoration created (works without EGL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/qt/bitcoin.cpp`:
- Around line 76-78: Wrap each Q_IMPORT_PLUGIN invocation
(Q_IMPORT_PLUGIN(QWaylandXdgShellIntegrationPlugin);
Q_IMPORT_PLUGIN(QWaylandEglClientBufferPlugin);
Q_IMPORT_PLUGIN(QWaylandBradientDecorationPlugin);) in its own compile-time
guard so the code only references qt_static_plugin_* symbols when that plugin is
actually built; e.g. surround each import with `#ifdef` <PLUGIN_SPECIFIC_DEFINE>
... `#endif` (use distinct defines per plugin as set by CMake via
target_compile_definitions for the corresponding plugin target) so the C++ build
matches the conditional linking in CMake.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c710a5bb-acaa-4a7c-a058-a8e629b67eb5
📒 Files selected for processing (4)
depends/packages/qt.mkdepends/patches/qt/qtwayland_decoration_no_egl_gate.patchsrc/qt/CMakeLists.txtsrc/qt/bitcoin.cpp
|
User peter@shugalev.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
psolstice
force-pushed
the
wayland-window-decorations
branch
from
22a58b8 to
a7c3e45
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
depends/packages/libXdmcp.mk (1)
1-23: 💤 Low valueConsider adding
postprocess_cmdsto remove.lafiles.This package definition is missing the cleanup step that other similar packages include. Libtool archive (
.la) files can cause linking issues due to hardcoded paths.Suggested addition for consistency with other packages
define $(package)_stage_cmds $(MAKE) DESTDIR=$($(package)_staging_dir) install endef + +define $(package)_postprocess_cmds + rm -f lib/*.la +endef🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@depends/packages/libXdmcp.mk` around lines 1 - 23, Add a postprocess step to remove libtool archive files by defining libXdmcp_postprocess_cmds (use the $(package)_postprocess_cmds naming pattern) and delete any *.la under the package staging dir (refer to $($(package)_staging_dir)) after install; implement it similarly to other packages so the .la files are removed from the staged install to avoid hardcoded path/linking issues.depends/packages/libxcb.mk (1)
41-41: ⚡ Quick winSplit the one-liner into readable multi-line steps.
The entire
postprocess_cmdsblock is a single long&&-chain, making it very difficult to audit, diff, or debug. Standard practice in the depends system is to use\continuations.♻️ Proposed refactor for readability
define $(package)_postprocess_cmds - rm -rf share lib/*.la && mkdir -p _xau _xdmcp && (cd _xau && ar x $(host_prefix)/lib/libXau.a) && (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a) && ar qc lib/libxcb.a _xau/*.o _xdmcp/*.o && ranlib lib/libxcb.a && rm -rf _xau _xdmcp && python3 -c "c=open('lib/pkgconfig/xcb.pc').read();open('lib/pkgconfig/xcb.pc','w').write(c.replace(' -lxcb\n',' -lxcb -lXau -lXdmcp\n'))" + rm -rf share lib/*.la && \ + mkdir -p _xau _xdmcp && \ + (cd _xau && ar x $(host_prefix)/lib/libXau.a) && \ + (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a) && \ + ar qc lib/libxcb.a _xau/*.o _xdmcp/*.o && \ + ranlib lib/libxcb.a && \ + rm -rf _xau _xdmcp && \ + python3 -c "c=open('lib/pkgconfig/xcb.pc').read(); \ + open('lib/pkgconfig/xcb.pc','w').write(c.replace(' -lxcb\n',' -lxcb -lXau -lXdmcp\n'))" endef🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@depends/packages/libxcb.mk` at line 41, The postprocess_cmds one-liner in depends/packages/libxcb.mk is unreadable; split it into multiple logical shell commands using line continuations (backslashes) so each step is its own line: remove share and lib/*.la, create _xau and _xdmcp, extract objects into _xau and _xdmcp with (cd _xau && ar x $(host_prefix)/lib/libXau.a) and (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a), archive and ranlib lib/libxcb.a, remove the temporary dirs, and finally run the python3 pkg-config edit that replaces ' -lxcb\n' with ' -lxcb -lXau -lXdmcp\n'; keep the exact commands/ordering (rm -rf share lib/*.la, mkdir -p _xau _xdmcp, ar x calls, ar qc, ranlib, rm -rf _xau _xdmcp, python3 -c ...) and ensure this is placed into postprocess_cmds in the same Makefile fragment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@depends/packages/libxcb.mk`:
- Line 41: The one-liner that edits lib/pkgconfig/xcb.pc using python3 -c
currently does a silent no-op when the pattern " -lxcb\n" isn't found; modify
that python snippet to verify the replacement took place (e.g., perform the
replace, compare original vs new, and if unchanged write nothing and exit with
non-zero/raise an exception or print an error), keeping the same pattern and
replacement strings (" -lxcb\n" -> " -lxcb -lXau -lXdmcp\n") so the build fails
fast if xcb.pc was not patched; locate the inline python in the shell command
that reads open('lib/pkgconfig/xcb.pc') and add the check/assert and non-zero
exit on failure.
---
Nitpick comments:
In `@depends/packages/libxcb.mk`:
- Line 41: The postprocess_cmds one-liner in depends/packages/libxcb.mk is
unreadable; split it into multiple logical shell commands using line
continuations (backslashes) so each step is its own line: remove share and
lib/*.la, create _xau and _xdmcp, extract objects into _xau and _xdmcp with (cd
_xau && ar x $(host_prefix)/lib/libXau.a) and (cd _xdmcp && ar x
$(host_prefix)/lib/libXdmcp.a), archive and ranlib lib/libxcb.a, remove the
temporary dirs, and finally run the python3 pkg-config edit that replaces '
-lxcb\n' with ' -lxcb -lXau -lXdmcp\n'; keep the exact commands/ordering (rm -rf
share lib/*.la, mkdir -p _xau _xdmcp, ar x calls, ar qc, ranlib, rm -rf _xau
_xdmcp, python3 -c ...) and ensure this is placed into postprocess_cmds in the
same Makefile fragment.
In `@depends/packages/libXdmcp.mk`:
- Around line 1-23: Add a postprocess step to remove libtool archive files by
defining libXdmcp_postprocess_cmds (use the $(package)_postprocess_cmds naming
pattern) and delete any *.la under the package staging dir (refer to
$($(package)_staging_dir)) after install; implement it similarly to other
packages so the .la files are removed from the staged install to avoid hardcoded
path/linking issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e048d602-b470-4227-8244-3f0892efbf4c
📒 Files selected for processing (13)
CMakeLists.txtdepends/packages/libXdmcp.mkdepends/packages/libxcb.mkdepends/packages/libxcb_util_image.mkdepends/packages/libxcb_util_keysyms.mkdepends/packages/libxcb_util_render.mkdepends/packages/libxcb_util_wm.mkdepends/packages/libxkbcommon.mkdepends/packages/packages.mkdepends/packages/qt.mkdepends/packages/wayland.mkdepends/patches/qt/qtbase_findxcb_image_aux.patchsrc/qt/CMakeLists.txt
✅ Files skipped from review due to trivial changes (3)
- depends/packages/packages.mk
- depends/patches/qt/qtbase_findxcb_image_aux.patch
- depends/packages/libxcb_util_image.mk
🚧 Files skipped from review as they are similar to previous changes (1)
- depends/packages/qt.mk
|
User peter@shugalev.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/qt/bitcoin.cpp (1)
76-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard optional Wayland static plugin imports with per-plugin defines.
These imports are unconditional once
HAVE_WAYLANDis set, but the corresponding plugin targets are conditionally linked in CMake. In static builds where one optional target is missing, this can produce unresolvedqt_static_plugin_*symbols at link time.Proposed fix
`#ifdef` HAVE_WAYLAND Q_IMPORT_PLUGIN(QWaylandIntegrationPlugin); Q_IMPORT_PLUGIN(QWaylandEglPlatformIntegrationPlugin); -Q_IMPORT_PLUGIN(QWaylandXdgShellIntegrationPlugin); -Q_IMPORT_PLUGIN(QWaylandEglClientBufferPlugin); -Q_IMPORT_PLUGIN(QWaylandBradientDecorationPlugin); +#ifdef HAVE_QT_WAYLAND_XDG_SHELL_INTEGRATION_PLUGIN +Q_IMPORT_PLUGIN(QWaylandXdgShellIntegrationPlugin); +#endif +#ifdef HAVE_QT_WAYLAND_EGL_CLIENT_BUFFER_PLUGIN +Q_IMPORT_PLUGIN(QWaylandEglClientBufferPlugin); +#endif +#ifdef HAVE_QT_WAYLAND_BRADIENT_DECORATION_PLUGIN +Q_IMPORT_PLUGIN(QWaylandBradientDecorationPlugin); +#endif `#endif`#!/bin/bash set -euo pipefail echo "== Optional Wayland imports in src/qt/bitcoin.cpp ==" rg -n -C2 'Q_IMPORT_PLUGIN\(QWayland(XdgShellIntegration|EglClientBuffer|BradientDecoration)Plugin\)' src/qt/bitcoin.cpp echo echo "== Plugin target existence checks in src/qt/CMakeLists.txt ==" rg -n -C3 'TARGET Qt6::QWayland(XdgShellIntegration|EglClientBuffer|BradientDecoration)Plugin' src/qt/CMakeLists.txt echo echo "== Compile defines exposed to bitcoin.cpp ==" rg -n -C3 'HAVE_QT_WAYLAND_(XDG_SHELL_INTEGRATION|EGL_CLIENT_BUFFER|BRADIENT_DECORATION)_PLUGIN' src/qt/CMakeLists.txt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/bitcoin.cpp` around lines 76 - 82, The unconditional Q_IMPORT_PLUGIN lines in bitcoin.cpp cause unresolved symbols when optional Wayland plugin targets are not linked; for each plugin import (QWaylandIntegrationPlugin, QWaylandEglPlatformIntegrationPlugin, QWaylandXdgShellIntegrationPlugin, QWaylandEglClientBufferPlugin, QWaylandBradientDecorationPlugin) wrap the Q_IMPORT_PLUGIN call in its own `#ifdef` using the per-plugin compile define used in CMake (e.g. HAVE_QT_WAYLAND_XDG_SHELL_INTEGRATION_PLUGIN, HAVE_QT_WAYLAND_EGL_CLIENT_BUFFER_PLUGIN, HAVE_QT_WAYLAND_BRADIENT_DECORATION_PLUGIN) so each plugin is only imported when its corresponding CMake target/define is present; leave the overall HAVE_WAYLAND guard around Wayland-related imports as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/qt/bitcoin.cpp`:
- Around line 76-82: The unconditional Q_IMPORT_PLUGIN lines in bitcoin.cpp
cause unresolved symbols when optional Wayland plugin targets are not linked;
for each plugin import (QWaylandIntegrationPlugin,
QWaylandEglPlatformIntegrationPlugin, QWaylandXdgShellIntegrationPlugin,
QWaylandEglClientBufferPlugin, QWaylandBradientDecorationPlugin) wrap the
Q_IMPORT_PLUGIN call in its own `#ifdef` using the per-plugin compile define used
in CMake (e.g. HAVE_QT_WAYLAND_XDG_SHELL_INTEGRATION_PLUGIN,
HAVE_QT_WAYLAND_EGL_CLIENT_BUFFER_PLUGIN,
HAVE_QT_WAYLAND_BRADIENT_DECORATION_PLUGIN) so each plugin is only imported when
its corresponding CMake target/define is present; leave the overall HAVE_WAYLAND
guard around Wayland-related imports as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1796d30f-c334-44fe-9548-49129839fd8f
📒 Files selected for processing (3)
contrib/debian/firo-qt.desktopsrc/qt/bitcoin.cppsrc/qt/bitcoin.qrc
✅ Files skipped from review due to trivial changes (1)
- src/qt/bitcoin.qrc
|
User peter@shugalev.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
depends/packages/libxcb.mk (1)
41-41: 💤 Low valueConsider splitting the postprocess chain for readability
The entire postprocess is a single 300+ character line. Make uses
&&continuations naturally; splitting across multiple lines with\significantly improves maintainability and debuggability.♻️ Proposed refactor
define $(package)_postprocess_cmds - rm -rf share lib/*.la && mkdir -p _xau _xdmcp && (cd _xau && ar x $(host_prefix)/lib/libXau.a) && (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a) && ar rc lib/libxcb.a _xau/*.o _xdmcp/*.o && ranlib lib/libxcb.a && rm -rf _xau _xdmcp && python3 -c "c=open('lib/pkgconfig/xcb.pc').read();n=c.replace(' -lxcb\n',' -lxcb -lXau -lXdmcp\n');assert n!=c,'xcb.pc patch failed: -lxcb not found at end of Libs line';open('lib/pkgconfig/xcb.pc','w').write(n)" + rm -rf share lib/*.la && \ + mkdir -p _xau _xdmcp && \ + (cd _xau && ar x $(host_prefix)/lib/libXau.a) && \ + (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a) && \ + ar rc lib/libxcb.a _xau/*.o _xdmcp/*.o && \ + ranlib lib/libxcb.a && \ + rm -rf _xau _xdmcp && \ + python3 -c "\ + c=open('lib/pkgconfig/xcb.pc').read(); \ + n=c.replace(' -lxcb\n',' -lxcb -lXau -lXdmcp\n'); \ + assert n!=c,'xcb.pc patch failed: -lxcb not found at end of Libs line'; \ + open('lib/pkgconfig/xcb.pc','w').write(n)" endef🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@depends/packages/libxcb.mk` at line 41, The long single-line postprocess command should be split into multiple chained shell commands using line continuations for readability while preserving behavior; break the sequence that starts with "rm -rf share lib/*.la && mkdir -p _xau _xdmcp" into separate lines (preserve the order of rm, mkdir, (cd _xau && ar x $(host_prefix)/lib/libXau.a), (cd _xdmcp && ar x $(host_prefix)/lib/libXdmcp.a), ar qc lib/libxcb.a _xau/*.o _xdmcp/*.o, ranlib lib/libxcb.a, rm -rf _xau _xdmcp, and the python3 -c patch of lib/pkgconfig/xcb.pc), joining them with "&& \\" so the commands and the python patch remain identical but are easier to read and debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@depends/packages/libxcb_util_image.mk`:
- Line 31: Replace the fragile archive step that uses "ar qc lib/libxcb-image.a
_util/*.o" and the "mkdir -p _util" setup: instead remove any stale temp objects
first by running "rm -rf _util && mkdir _util" before extracting, and use "ar rc
lib/libxcb-image.a _util/*.o" so archive members are replaced (not appended) on
retries; ensure the final cleanup "rm -rf _util" still runs only after
successful steps to avoid leaving stale objects for subsequent runs and
reference the archive name "lib/libxcb-image.a" and temp directory "_util" when
making these changes.
In `@depends/packages/libxcb.mk`:
- Line 41: The command that builds lib/libxcb.a currently uses "ar qc" which
appends object members and can produce duplicate entries; edit the shell
pipeline that contains "ar qc lib/libxcb.a _xau/*.o _xdmcp/*.o" (the line that
also does ar x into _xau/_xdmcp and ranlib) and change "ar qc" to "ar rc" so
members are replaced rather than appended, keeping the subsequent ranlib step
intact.
---
Nitpick comments:
In `@depends/packages/libxcb.mk`:
- Line 41: The long single-line postprocess command should be split into
multiple chained shell commands using line continuations for readability while
preserving behavior; break the sequence that starts with "rm -rf share lib/*.la
&& mkdir -p _xau _xdmcp" into separate lines (preserve the order of rm, mkdir,
(cd _xau && ar x $(host_prefix)/lib/libXau.a), (cd _xdmcp && ar x
$(host_prefix)/lib/libXdmcp.a), ar qc lib/libxcb.a _xau/*.o _xdmcp/*.o, ranlib
lib/libxcb.a, rm -rf _xau _xdmcp, and the python3 -c patch of
lib/pkgconfig/xcb.pc), joining them with "&& \\" so the commands and the python
patch remain identical but are easier to read and debug.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6872645f-1b87-4c8c-9793-929eb5138fad
📒 Files selected for processing (5)
depends/packages/libXdmcp.mkdepends/packages/libxcb.mkdepends/packages/libxcb_util_image.mksrc/qt/CMakeLists.txtsrc/qt/bitcoin.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- depends/packages/libXdmcp.mk
- src/qt/bitcoin.cpp
- src/qt/CMakeLists.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR intention
Add patches to the build scripts to fix the problem on non-EGL wayland sessions