Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClassic UI theme rendering now supports SVG assets, Cairo surfaces, and mesh patterns. Theme painting uses region-based rendering and device scaling, while input, menu, tray, and window paths adopt the updated painting API and composition behavior. ChangesClassic UI SVG rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ThemeConfig
participant ThemeImage
participant librsvg
participant Theme
participant ClassicWindow
ThemeConfig->>ThemeImage: load SVG or raster asset
ThemeImage->>librsvg: parse SVG document
librsvg-->>ThemeImage: return SVG handle and dimensions
ClassicWindow->>Theme: request painting for destination region
Theme->>ThemeImage: paintRegion source into destination
ThemeImage-->>ClassicWindow: render image through Cairo
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/ui/classic/theme.cpp (1)
251-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSVG parse errors are discarded.
rsvg_handle_new_from_stream_syncis passed a nullGError**, so a malformed theme SVG fails silently. Passing aGErrorand logging viaCLASSICUI_ERROR()would make theme authoring issues diagnosable.🤖 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 `@src/ui/classic/theme.cpp` around lines 251 - 275, Update loadSvg’s rsvg_handle_new_from_stream_sync call to capture parse failures in a GError, and log the error through CLASSICUI_ERROR() before returning std::nullopt when handle creation fails. Preserve the existing successful-load and other failure paths.src/ui/classic/xcbmenu.cpp (1)
536-537: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-lookup shadows the outer
separator.
separatoris already bound at line 433; re-callingloadBackgroundper separator item inside the paint loop adds a hash lookup and shadows the outer name.♻️ Proposed cleanup
if (item.isSeparator_) { - const ThemeImage &separator = - theme.loadBackground(*theme.menu->separator); theme.paint(c, *theme.menu->separator, item.layoutX_, item.layoutY_,🤖 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 `@src/ui/classic/xcbmenu.cpp` around lines 536 - 537, In the separator painting loop, remove the inner `separator` declaration and reuse the existing outer `separator` binding established earlier in the function. Avoid calling `theme.loadBackground` for each separator item while preserving the current rendering behavior.src/ui/classic/theme.h (1)
281-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
isImage_member.
isImage_is only declared; there are no assignments or readers left insrc/ui/classic, so remove the dead state member.🤖 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 `@src/ui/classic/theme.h` around lines 281 - 287, Remove the unused isImage_ member from the class declaration near currentText_ and size_. Leave the remaining state fields and ImageVariant alias unchanged.
🤖 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 `@CMakeLists.txt`:
- Line 167: The librsvg discovery and classicui linkage use inconsistent feature
contracts. In CMakeLists.txt at lines 167-167 and src/ui/classic/CMakeLists.txt
at lines 45-45, either require librsvg whenever classicui is enabled under the
same ENABLE_X11/ENABLE_WAYLAND guard, or introduce an explicit optional-librsvg
condition and apply it consistently to both pkg_check_modules(Rsvg) and the
PkgConfig::Rsvg link.
In `@src/ui/classic/theme.cpp`:
- Around line 636-685: Update ThemeImage::paintRegion to return immediately when
sourceWidth or sourceHeight is zero or otherwise non-positive, before any branch
computes scaling or translation. Preserve the existing rendering behavior for
valid source regions across the CairoSurface, Svg, and Pattern paths.
---
Nitpick comments:
In `@src/ui/classic/theme.cpp`:
- Around line 251-275: Update loadSvg’s rsvg_handle_new_from_stream_sync call to
capture parse failures in a GError, and log the error through CLASSICUI_ERROR()
before returning std::nullopt when handle creation fails. Preserve the existing
successful-load and other failure paths.
In `@src/ui/classic/theme.h`:
- Around line 281-287: Remove the unused isImage_ member from the class
declaration near currentText_ and size_. Leave the remaining state fields and
ImageVariant alias unchanged.
In `@src/ui/classic/xcbmenu.cpp`:
- Around line 536-537: In the separator painting loop, remove the inner
`separator` declaration and reuse the existing outer `separator` binding
established earlier in the function. Avoid calling `theme.loadBackground` for
each separator item while preserving the current rendering behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f1e4c1f-c52c-4208-9efc-213bcc3f5a11
⛔ Files ignored due to path filters (16)
src/ui/classic/themes/default-dark/arrow.pngis excluded by!**/*.pngsrc/ui/classic/themes/default-dark/arrow.svgis excluded by!**/*.svgsrc/ui/classic/themes/default-dark/next.pngis excluded by!**/*.pngsrc/ui/classic/themes/default-dark/next.svgis excluded by!**/*.svgsrc/ui/classic/themes/default-dark/prev.pngis excluded by!**/*.pngsrc/ui/classic/themes/default-dark/prev.svgis excluded by!**/*.svgsrc/ui/classic/themes/default-dark/radio.pngis excluded by!**/*.pngsrc/ui/classic/themes/default-dark/radio.svgis excluded by!**/*.svgsrc/ui/classic/themes/default/arrow.pngis excluded by!**/*.pngsrc/ui/classic/themes/default/arrow.svgis excluded by!**/*.svgsrc/ui/classic/themes/default/next.pngis excluded by!**/*.pngsrc/ui/classic/themes/default/next.svgis excluded by!**/*.svgsrc/ui/classic/themes/default/prev.pngis excluded by!**/*.pngsrc/ui/classic/themes/default/prev.svgis excluded by!**/*.svgsrc/ui/classic/themes/default/radio.pngis excluded by!**/*.pngsrc/ui/classic/themes/default/radio.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
CMakeLists.txtsrc/ui/classic/CMakeLists.txtsrc/ui/classic/inputwindow.cppsrc/ui/classic/inputwindow.hsrc/ui/classic/theme.cppsrc/ui/classic/theme.hsrc/ui/classic/themes/default-dark/CMakeLists.txtsrc/ui/classic/themes/default-dark/theme-dark.conf.insrc/ui/classic/themes/default/CMakeLists.txtsrc/ui/classic/themes/default/theme.conf.insrc/ui/classic/waylandinputwindow.cppsrc/ui/classic/xcbinputwindow.cppsrc/ui/classic/xcbmenu.cppsrc/ui/classic/xcbtraywindow.cppsrc/ui/classic/xcbwindow.cpp
| pkg_check_modules(Cairo IMPORTED_TARGET cairo) | ||
| pkg_check_modules(Pango IMPORTED_TARGET pango pangocairo) | ||
| pkg_check_modules(GdkPixbuf IMPORTED_TARGET gdk-pixbuf-2.0) | ||
| pkg_check_modules(Rsvg IMPORTED_TARGET librsvg-2.0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(CMakeLists.txt)$' | sed -n '1,200p'
echo "== root CMake snippets =="
if [ -f CMakeLists.txt ]; then
wc -l CMakeLists.txt
sed -n '130,190p' CMakeLists.txt
fi
echo "== classic CMake snippets =="
if [ -f src/ui/classic/CMakeLists.txt ]; then
wc -l src/ui/classic/CMakeLists.txt
sed -n '1,90p' src/ui/classic/CMakeLists.txt
fi
echo "== occurrences of Rsvg/Rsvg targets and feature guards =="
rg -n "librsvg|PkgConfig::Rsvg|ENABLE_WAYLAND|Classic|classicui|add_fcitx5_addon\\(" .Repository: fcitx/fcitx5
Length of output: 42558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/ui/CMake.txt context =="
if [ -f src/ui/CMakeLists.txt ]; then
wc -l src/ui/CMakeLists.txt
sed -n '1,120p' src/ui/CMakeLists.txt
fi
echo "== classicui guarded source/config sections =="
rg -n "ENABLE_CLASSICUI|CLASSICUI|classicui|ENABLE_WAYLAND|ENABLE_X11" src/ui src/ui/classic CMakeLists.txt test -g '*.cmake' -g 'CMakeLists.txt' | sed -n '1,220p'
echo "== deterministic CMake-like behavior probe =="
python3 - <<'PY'
# Model the relevant CMake dependency-discovery/linking behavior described by the files.
cases = [
{"name": "ENABLE_WAYLAND=On, librsvg absent", "ENABLE_WAYLAND": 1, "ENABLE_X11": 0, "libs_availability": {"Rsvg": False}},
{"name": "ENABLE_X11=On only, librsvg absent", "ENABLE_WAYLAND": 0, "ENABLE_X11": 1, "libs_availability": {"Rsvg": False}},
{"name": "both enabled, librsvg present", "ENABLE_WAYLAND": 1, "ENABLE_X11": 1, "libs_availability": {"Rsvg": True}},
]
for c in cases:
discovery_guard = c["ENABLE_X11"] or c["ENABLE_WAYLAND"]
has_pkg_cfg_target = discovery_guard and c["libs_availability"]["Rsvg"]
build_block_guard = discovery_guard
link_has_target = build_block_guard
status = "FAIL at config/link" if (build_block_guard and not has_pkg_cfg_target and link_has_target) else "OK"
print(f"{c['name']}: discovery_guard={discovery_guard}, pkg_target_available={has_pkg_cfg_target}, classicui_block_added={build_block_guard}, link_targets_resolved={link_has_target} -> {status}")
PYRepository: fcitx/fcitx5
Length of output: 5131
Keep librsvg discovery and classicui linking on the same feature contract.
pkg_check_modules(Rsvg IMPORTED_TARGET librsvg-2.0) is guarded only by ENABLE_X11 OR ENABLE_WAYLAND, but classicui still links PkgConfig::Rsvg unconditionally. Builds with Wayland enabled but without librsvg will fail configuration/connection. Either make this dependency required under the same guard as classicui, or gate both discovery and the PkgConfig::Rsvg link step behind an explicit optional-librsvg path.
📍 Affects 2 files
CMakeLists.txt#L167-L167(this comment)src/ui/classic/CMakeLists.txt#L45-L45
🤖 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 `@CMakeLists.txt` at line 167, The librsvg discovery and classicui linkage use
inconsistent feature contracts. In CMakeLists.txt at lines 167-167 and
src/ui/classic/CMakeLists.txt at lines 45-45, either require librsvg whenever
classicui is enabled under the same ENABLE_X11/ENABLE_WAYLAND guard, or
introduce an explicit optional-librsvg condition and apply it consistently to
both pkg_check_modules(Rsvg) and the PkgConfig::Rsvg link.
Source: Coding guidelines
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores