Skip to content

rog-control-center: AC-style frontend redesign - #230

Open
NB-Group wants to merge 6 commits into
OpenGamingCollective:mainfrom
NB-Group:main
Open

rog-control-center: AC-style frontend redesign#230
NB-Group wants to merge 6 commits into
OpenGamingCollective:mainfrom
NB-Group:main

Conversation

@NB-Group

@NB-Group NB-Group commented Jul 28, 2026

Copy link
Copy Markdown

rog-control-center: AC-style frontend redesign

So this redoes the whole rog-control-center UI to look like Armoury Crate. New component lib, a dashboard home page, AC styling on every page. It's purely the UI layer — I didn't touch how it talks to asusd, except for one actual bug in asusd I tripped over (fixed it, see the bottom). No new D-Bus interfaces.

The look lives in one file. ui/widgets/rog.slint is a pile of self-drawn AC-styled primitives — RogCard, RogSlider (grey track + red fill + white/red handle), RogToggle, RogModeSelector, the dashboard bits (RogMetric/RogReading/RogStatRow), RogTabBar, RogEffectButton, RogDisplayModeCard, the slash accent, the meter. One Theme global owns every colour, so pages never hardcode hex. Pages just compose these.

How a page talks to asusd — same wiring that was already in src/ui/mod.rs, just pointed at the new globals. Three macros carry it:

  • set_ui_props_async! reads a property into the Slint global once at startup
  • set_ui_callbacks! is both directions — the UI callback writes through the proxy, and a receive_*_changed stream writes external changes back
  • the system page has its own (init_property / setup_callback / setup_external / setup_minmax_external) for the asus-armoury firmware attrs, because those are min/max/current, not plain values

setup_window() mounts a page's setup only if its xyz.ljones.* iface exists, and sets sidebar_items_available so the sidebar hides pages the device doesn't have.

Then per page:

  • Home (new) — product name from DMI, CPU and GPU model strings off the platform probe, Operation Mode buttons built off platform_profile_indexes (the profiles the box actually has, not some fixed AC set); click fires cb_platform_profile(enum). Live readouts — CPU clock, GPU clock via NVML, temps, fans, RAM — all come off the system poll.
  • System — platform profile, PPT, charge limit, every asus-armoury attr (panel overdrive, boot sound, MCU powersave, mini-LED...), and the 2s telemetry poll that only runs while the window is open.
  • Aura — mode/colour/zone/speed/direction all ride together in led_mode_data. The speed slider just stamps the new speed on the active effect and reuses the one apply path; brightness is its own binding.
  • Slash — mode is read with a short retry because a single read right after start loses the race with asusd. Other nine props are stock. Reset icon drops interval/brightness back to defaults.
  • Fan curves — reads and writes the nine curves (balanced/performance/quiet × cpu/mid/gpu) over FanCurvesProxy, plus per-profile reset.
  • GPU — mode switch writes the asus-armoury dgpu_disable / gpu_mux_mode attrs, it's not a method call. Also XG Mobile LED, nvidia-powerd-on-battery, reserved iGPU memory.
  • Battery — no setup of its own, reads the SystemPageData fields the system poll fills.

All nine pages stay mounted in main_window.slint. Switching is an opacity crossfade (180ms); inactive pages go visible: false so they cost nothing to draw.

Language switching. App Settings has a picker (English / 简体中文); picking one writes config.language and a Reload relaunches the window so @tr() re-resolves — no manual restart. The catch: main.rs pushes the configured language into gettext via a setlocale(LC_ALL) FFI call before init_translations, otherwise the new process still renders the old locale (gettext falls back to LANG). The reload spawns a fresh process flagged ROGCC_NO_SINGLE_INSTANCE and quits the old one, so the single-instance guard doesn't swallow the new window. zh_CN updated to cover the new UI.

One real asusd bug. asusd/src/aura_slash/trait_impls.rs: the Slash mode() getter was returning the display interval (a u8) instead of the display mode, so the UI always opened on Static. Two-line fix. Honestly worth landing even without the UI work.

The one scaffold left. The GPU page has a commented process-list block — asusd exposes no GPU process listing, so it's scaffolded for later rather than left as a dead button in the UI.

Comment thread rog-control-center/src/ui/mod.rs Outdated

@scardracs scardracs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked the patch a bit more and these are the things that personally I'd look again

Comment thread rog-control-center/src/main.rs Outdated
Comment thread rog-control-center/src/ui/setup_slash.rs
Comment thread rog-control-center/src/main.rs Outdated
Comment thread rog-control-center/src/ui/setup_system.rs Outdated
Comment thread rog-control-center/src/ui/mod.rs
Comment thread rog-control-center/ui/main_window.slint Outdated
Comment thread rog-control-center/ui/main_window.slint
Comment thread rog-control-center/src/ui/mod.rs
Comment thread rog-platform/src/gpu_pci.rs Outdated
@scardracs

scardracs commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Please rewrite the commits and fixup them in a logical way. Writing down that you were fixing my review is not useful on the long run. Same with other commits

@luytan

luytan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5fbcfcef-680f-4bd2-be05-1b3110c092e3

📥 Commits

Reviewing files that changed from the base of the PR and between 9e173e2 and 47cc2fa.

📒 Files selected for processing (14)
  • rog-control-center/src/ui/mod.rs
  • rog-control-center/translations/zh_CN/rog-control-center.po
  • rog-control-center/ui/globals.slint
  • rog-control-center/ui/pages/about.slint
  • rog-control-center/ui/pages/aura.slint
  • rog-control-center/ui/pages/battery.slint
  • rog-control-center/ui/pages/fans.slint
  • rog-control-center/ui/pages/gpu.slint
  • rog-control-center/ui/pages/home.slint
  • rog-control-center/ui/pages/system.slint
  • rog-control-center/ui/widgets/common.slint
  • rog-control-center/ui/widgets/graph.slint
  • rog-control-center/ui/widgets/rog.slint
  • rog-control-center/ui/widgets/sidebar.slint
 ______________________________________________________________________________________________________
< What one programmer can do in one month, two programmers can do in two months. - Frederick P. Brooks >
 ------------------------------------------------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
📝 Walkthrough

Walkthrough

The control center receives a shared ROG-themed UI foundation, redesigned pages, language selection and reload support, GPU telemetry, updated Aura controls, and a corrected Slash mode DBus contract with startup retries.

Changes

Control-center redesign

Layer / File(s) Summary
Shared theme, widgets, and window shell
rog-control-center/ui/globals.slint, rog-control-center/ui/widgets/*, rog-control-center/ui/main_window.slint
Adds shared theme tokens and ROG widgets, rewrites sidebar navigation, updates graph/common styling, and changes page switching to opacity-based rendering.
Dashboard and settings pages
rog-control-center/ui/pages/home.slint, rog-control-center/ui/pages/about.slint, rog-control-center/ui/pages/app_settings.slint, rog-control-center/ui/pages/battery.slint, rog-control-center/ui/pages/fans.slint, rog-control-center/ui/pages/gpu.slint, rog-control-center/ui/pages/anime.slint
Rebuilds the listed pages around RogPage, themed cards, shared controls, dashboard metrics, language settings, battery controls, fan selectors, and GPU mode cards.

Aura and Slash integration

Layer / File(s) Summary
Aura controls and apply callbacks
rog-control-center/ui/types/aura_types.slint, rog-control-center/ui/pages/aura.slint, rog-control-center/src/ui/setup_aura.rs
Adds Aura speed callbacks, effect controls, sliders, modal power settings, and routes speed changes through the existing apply pipeline.
Slash mode contract and page flow
asusd/src/aura_slash/trait_impls.rs, rog-control-center/src/ui/setup_slash.rs, rog-control-center/ui/pages/slash.slint
Exposes SlashMode, retries startup reads, and adds the animated preview, sliders, basic effects, and visibility controls.

Runtime integration

Layer / File(s) Summary
System identity and GPU telemetry
rog-platform/src/gpu_pci.rs, rog-control-center/src/ui/setup_system.rs, rog-control-center/ui/pages/system.slint
Adds GPU frequency probing and publishes GPU frequency and DMI product name to system page state.
Language selection and locale initialization
rog-control-center/src/config.rs, rog-control-center/src/main.rs, rog-control-center/src/ui/mod.rs, rog-control-center/ui/pages/app_settings.slint, rog-control-center/translations/zh_CN/*
Stores the selected language, initializes locale variables before UI startup, discovers available locales, persists changes, reloads the process, and refreshes Chinese catalogs.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description summarizes the redesign well, but it omits required template sections like Fixes #, tested hardware/environment, and the verification checklist. Add the template sections for Fixes #, ASUS model, distro, kernel, and the verification checklist items; fill any applicable TODOs.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: an AC-style frontend redesign.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 7 minutes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

🧹 Nitpick comments (9)
rog-control-center/ui/pages/aura.slint (3)

352-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Speed range comment in aura_types.slint says 0..3; this slider writes 0..2.

The slider maps to 0..2, matching speed_names (3 entries) and rog_aura speed levels. The new cb_speed doc comment in rog-control-center/ui/types/aura_types.slint (line 77) states "0..3" — worth correcting so future callers don't send an out-of-range value.

🤖 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 `@rog-control-center/ui/pages/aura.slint` around lines 352 - 366, Update the
cb_speed documentation in aura_types.slint to state that valid speed values are
0..2, matching the range produced by the ta-speed TouchArea and the three speed
levels in speed_names; do not change the slider behavior.

51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Give content an explicit height.

Without height, content defaults to the full parent height while being offset by titlebar.height, so it overhangs the bottom by 52px (the trailing padding-bottom: 24px is effectively clipped). slash.slint sets this explicitly.

♻️ Proposed change
     content := Rectangle {
         y: titlebar.height;
         width: 100%;
+        height: root.height - titlebar.height;
🤖 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 `@rog-control-center/ui/pages/aura.slint` around lines 51 - 53, Update the
content Rectangle in the aura page to set an explicit height equal to the parent
height minus titlebar.height, preserving the existing y offset and ensuring the
bottom padding remains within the available area.

459-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Three near-identical modal shells — extract one.

The New/Tuf/Old blocks repeat the same overlay geometry, drop shadow, header text and close button verbatim; only the zone widget and the legacy zone section differ. Extracting a local AuraModal component (title + close callback + @children) would cut ~140 duplicated lines and keep styling in sync.

🤖 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 `@rog-control-center/ui/pages/aura.slint` around lines 459 - 673, Extract the
duplicated modal shell from the New, Tuf, and Old power-settings blocks into a
reusable local AuraModal component containing the shared geometry, styling,
header, title, and close-button behavior, with a children slot for page-specific
content. Replace each device-specific Rectangle shell with AuraModal and retain
only its distinct zone widget and legacy text/zone bindings inside the child
content.
rog-control-center/src/ui/setup_slash.rs (1)

62-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Skip the sleep after the last attempt.

The current loop sleeps 300ms after attempt 5 even though no further read follows, delaying the error log (and per-device iteration) needlessly.

♻️ Proposed change
                     Err(e) => {
                         log::warn!("slash mode read attempt {} failed: {e}", attempt + 1);
-                        tokio::time::sleep(std::time::Duration::from_millis(300)).await;
+                        if attempt < 4 {
+                            tokio::time::sleep(std::time::Duration::from_millis(300)).await;
+                        }
                     }
🤖 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 `@rog-control-center/src/ui/setup_slash.rs` around lines 62 - 74, Update the
retry loop around slash.mode().await so tokio::time::sleep is performed only
when another attempt remains; retain the existing warning and retry behavior,
but skip the 300ms delay after the fifth and final attempt.
rog-control-center/ui/main_window.slint (1)

97-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Home page sizing diverges from every other page.

PageHome is capped at 1100px, centered, and uses root.height, while all other pages take the full remaining width and root.height + 12px. Switching pages will visibly shift/resize content. Worth aligning on one rule (either cap all pages or none) and dropping the stray + 12px.

🤖 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 `@rog-control-center/ui/main_window.slint` around lines 97 - 161, Align
PageHome with the sizing behavior of the other page components: use the full
remaining width, root.height + 12px height, and remove its 1100px cap and
centering x calculation. Keep its existing opacity and visibility conditions
unchanged so switching pages uses consistent dimensions without shifts.
rog-control-center/ui/pages/battery.slint (1)

133-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the health threshold colours into Theme.

#4CAF50 / #FF9E1B / #FF1929 are the only raw hex values on this page; centralizing them as Theme.status-good/warn/bad keeps the new token system authoritative.

🤖 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 `@rog-control-center/ui/pages/battery.slint` around lines 133 - 140, Update the
battery health Text color expression in the battery health display to use
Theme.status-good, Theme.status-warn, and Theme.status-bad instead of the raw
hex values, preserving the existing >80 and >50 thresholds and fallback
behavior.
rog-control-center/ui/pages/gpu.slint (1)

140-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Drop the 79-line commented-out Processes block.

The instructions for re-enabling are useful, but they belong in an issue or a short comment, not in ~79 lines of dead markup that will silently rot against Theme/widget changes. Want me to open a tracking issue with this scaffolding attached so it can be deleted here?

🤖 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 `@rog-control-center/ui/pages/gpu.slint` around lines 140 - 218, Remove the
entire commented-out Processes markup block under the Process List Section in
the GPU page, including its re-enablement instructions and dead widget
scaffolding. Keep only a brief comment if future tracking context is required;
do not retain the 79-line commented UI block.
rog-control-center/ui/pages/about.slint (2)

158-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Link text is styled as a link but isn't actionable.

Theme.rog-blue on these two URLs signals clickability; nothing opens them. Either add a TouchArea wired to an xdg-open callback or drop the link colouring.

🤖 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 `@rog-control-center/ui/pages/about.slint` around lines 158 - 164, Update the
URL Text block in the about page so the displayed URLs are actionable: add a
TouchArea that invokes the existing callback or mechanism to open the URLs
through xdg-open, while preserving the current link styling; if no suitable open
callback exists, remove Theme.rog-blue from this text instead.

104-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Split the feature list into separate @tr strings.

Packing five features plus embedded \n into a single translatable unit means a translator must reproduce the exact newline structure, and any change to one line invalidates the whole entry in the .po catalogue. Separate Text elements (or a for over individually translated strings) localize far better.

🤖 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 `@rog-control-center/ui/pages/about.slint` around lines 104 - 111, The
about-page feature list currently uses one `@tr` string containing five lines and
embedded newlines. Update the feature-list Text UI around the existing
“Performance” content to translate each feature as a separate `@tr` string, using
separate Text elements or an equivalent per-item iteration while preserving the
current styling and display order.
🤖 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 `@rog-control-center/src/main.rs`:
- Around line 189-206: Move the locale setup block identified by its config
language lookup and libc::setlocale call so it executes before Runtime::new()
constructs the Tokio runtime. Preserve the existing language fallback, CString
handling, and LC_ALL behavior; do not perform this process-global locale
mutation after runtime or worker-thread initialization.

In `@rog-control-center/src/ui/mod.rs`:
- Around line 211-236: Update available_languages so the returned language list
always includes the "en" SharedString, even when other catalogs such as zh_CN
are discovered. Preserve the existing deduplication and sorting behavior, and
keep the fallback for an otherwise empty language set.

In `@rog-control-center/ui/pages/about.slint`:
- Around line 67-72: Replace the hardcoded version text in the About page with a
global property supplied by Rust using env!("CARGO_PKG_VERSION"). Follow the
existing SystemPageData pattern for exposing identity data, and bind the Text
element’s version display to that property so release version bumps remain
synchronized.

In `@rog-control-center/ui/pages/app_settings.slint`:
- Around line 210-229: The hand-rolled action rectangles lack keyboard focus,
activation, and visual interaction states. Add a themed RogButton primitive in
widgets/rog.slint with FocusScope, Return/Space key handling, and pressed/hover
states, then replace the Reload Window action at
rog-control-center/ui/pages/app_settings.slint:210-229 and the Set up/Configure
shortcut action at rog-control-center/ui/pages/app_settings.slint:172-192;
replace the Apply and Factory Default actions at
rog-control-center/ui/pages/fans.slint:56-87, preserving root.tab_enabled gating
and disabled colors.

In `@rog-control-center/ui/pages/aura.slint`:
- Around line 386-395: Update the brightness-dot condition in the AuraPageData
brightness indicator so brightness 0 lights no dots, while positive brightness
values retain the intended number of lit dots; adjust the comparison in the `for
bi in 4` Rectangle block without changing the layout or styling.
- Around line 141-211: Replace the four hardcoded effect buttons in the
AuraPageData layout with controls generated from the existing dynamic mode list,
pairing each displayed mode with its corresponding supported_basic_modes entry
and mode_names label. Ensure selection and apply_effect use the actual mode
index, preserve per-mode artwork where applicable, and expose supported modes
beyond the first four.

In `@rog-control-center/ui/pages/battery.slint`:
- Around line 73-76: Wire the `RogToggle` for
`SystemPageData.charge_control_enabled` to the backend by adding the
enable/disable callback exposed by `setup_system.rs`. Ensure toggling `Enable
Charge Limit` invokes that callback with the new state, while preserving the
existing two-way binding and slider behavior.

In `@rog-control-center/ui/pages/fans.slint`:
- Around line 73-81: The “Factory Default (all fans)” button uses a fixed 160px
width that cannot accommodate the label and longer translations. Update the
surrounding Rectangle to size from its content using an appropriate min-width
and horizontal padding, removing the fixed width while preserving the existing
height and styling.

In `@rog-control-center/ui/pages/home.slint`:
- Around line 117-121: Replace the gpu-metric RogMetric’s visible property with
conditional instantiation using if SystemPageData.has_dgpu, so the card is
omitted entirely when no discrete GPU exists. Keep the GPU card’s existing
properties unchanged and ensure its layout slot collapses on iGPU-only systems.

In `@rog-control-center/ui/pages/slash.slint`:
- Around line 562-592: Update the mode ComboBox and both SlashSlider controls in
the SlashPageData effect-controls block to remain enabled independently of
SlashPageData.enabled, allowing mode, interval, and brightness changes while
lighting is off; preserve their existing bindings and callbacks.
- Around line 485-534: The theme-preview block’s “Change Theme” control and the
nearby “?” help badge are rendered as actionable UI without behavior. Either add
working click handling for both using the page’s existing interaction patterns,
or remove these controls from the change so users are not presented with
non-functional actions.
- Around line 174-185: Add focus and keyboard support to all hand-rolled
sliders: update the reusable SlashSlider in
rog-control-center/ui/pages/slash.slint lines 174-185 so both slash sliders
support focus, arrow-key value stepping, and the existing release callback;
update the speed slider in rog-control-center/ui/pages/aura.slint lines 352-366
to step via keyboard and invoke AuraPageData.cb_speed; update the brightness
slider in rog-control-center/ui/pages/aura.slint lines 423-435 similarly using
AuraPageData.cb_brightness, reusing a shared slider widget where practical.

In `@rog-control-center/ui/widgets/common.slint`:
- Around line 5-8: Update the RogItem styling and its child control color
configuration so the fixed Theme.bg-card background remains readable under light
color schemes; either force the containing window’s Palette.color-scheme to dark
or replace the affected SystemSlider, SystemToggle*, and SystemDropdown
foreground bindings with Theme.text-primary.

In `@rog-control-center/ui/widgets/rog.slint`:
- Around line 149-284: Add keyboard focus, activation, and accessibility support
to the shared custom controls, including RogSlider, RogToggle, RogToggleInt,
RogModeSelector, RogTabBar, RogIconCard, and RogEffectButton. Give each control
an appropriate FocusScope, accessible-role and accessible-label, and key-pressed
handling that preserves slider arrow adjustments and toggle/button space or
enter activation while respecting enabled state. Reuse the existing value,
selection, and callback behavior rather than duplicating control logic.
- Around line 203-234: Update the Slider’s set-from-x function to map pointer
coordinates over the handle’s reduced travel width, matching the handle position
calculation using track.width - 16px; clamp the adjusted coordinate to that
range so clicking the handle center and reaching both endpoints produce the
displayed minimum and maximum values.

In `@rog-platform/src/gpu_pci.rs`:
- Around line 723-758: Update the GPU clock lookup around the NVML, DRM, and
hwmon fallback paths so every candidate is verified against the actual dGPU
before returning telemetry. Use the dGPU’s existing Device record and associated
PCI/sysfs identity to select the matching NVML device, DRM card, and amdgpu
hwmon node; do not rely on device index 0 or the first matching directory entry.
Preserve the existing clock parsing and fallback order once candidates are
filtered.
- Around line 756-758: Update the freq1_input parsing path to convert the parsed
hertz value to megahertz before returning it from the _mhz API. In the visible
read_to_string and parse::<f32>() flow, divide v by 1_000_000.0 while preserving
the existing Some return behavior.

---

Nitpick comments:
In `@rog-control-center/src/ui/setup_slash.rs`:
- Around line 62-74: Update the retry loop around slash.mode().await so
tokio::time::sleep is performed only when another attempt remains; retain the
existing warning and retry behavior, but skip the 300ms delay after the fifth
and final attempt.

In `@rog-control-center/ui/main_window.slint`:
- Around line 97-161: Align PageHome with the sizing behavior of the other page
components: use the full remaining width, root.height + 12px height, and remove
its 1100px cap and centering x calculation. Keep its existing opacity and
visibility conditions unchanged so switching pages uses consistent dimensions
without shifts.

In `@rog-control-center/ui/pages/about.slint`:
- Around line 158-164: Update the URL Text block in the about page so the
displayed URLs are actionable: add a TouchArea that invokes the existing
callback or mechanism to open the URLs through xdg-open, while preserving the
current link styling; if no suitable open callback exists, remove Theme.rog-blue
from this text instead.
- Around line 104-111: The about-page feature list currently uses one `@tr` string
containing five lines and embedded newlines. Update the feature-list Text UI
around the existing “Performance” content to translate each feature as a
separate `@tr` string, using separate Text elements or an equivalent per-item
iteration while preserving the current styling and display order.

In `@rog-control-center/ui/pages/aura.slint`:
- Around line 352-366: Update the cb_speed documentation in aura_types.slint to
state that valid speed values are 0..2, matching the range produced by the
ta-speed TouchArea and the three speed levels in speed_names; do not change the
slider behavior.
- Around line 51-53: Update the content Rectangle in the aura page to set an
explicit height equal to the parent height minus titlebar.height, preserving the
existing y offset and ensuring the bottom padding remains within the available
area.
- Around line 459-673: Extract the duplicated modal shell from the New, Tuf, and
Old power-settings blocks into a reusable local AuraModal component containing
the shared geometry, styling, header, title, and close-button behavior, with a
children slot for page-specific content. Replace each device-specific Rectangle
shell with AuraModal and retain only its distinct zone widget and legacy
text/zone bindings inside the child content.

In `@rog-control-center/ui/pages/battery.slint`:
- Around line 133-140: Update the battery health Text color expression in the
battery health display to use Theme.status-good, Theme.status-warn, and
Theme.status-bad instead of the raw hex values, preserving the existing >80 and
>50 thresholds and fallback behavior.

In `@rog-control-center/ui/pages/gpu.slint`:
- Around line 140-218: Remove the entire commented-out Processes markup block
under the Process List Section in the GPU page, including its re-enablement
instructions and dead widget scaffolding. Keep only a brief comment if future
tracking context is required; do not retain the 79-line commented UI block.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c32cd00-ddeb-4350-a31a-3af721c71f78

📥 Commits

Reviewing files that changed from the base of the PR and between 5055001 and 347048f.

⛔ Files ignored due to path filters (36)
  • rog-control-center/ui/images/effect-breathing.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-cycle.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-flash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-static.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu-fill.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-dedicated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-hybrid.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-integrated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-hz.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-moon.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-speaker.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-sync.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-temp.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-touchpad.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/laptop-placeholder.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-manual.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-performance.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-silent.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-turbo.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-about.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-apps.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-home.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-keyboard.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-matrix.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-slash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-bar.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-device-shell.svg is excluded by !**/*.svg
📒 Files selected for processing (28)
  • asusd/src/aura_slash/trait_impls.rs
  • rog-control-center/Cargo.toml
  • rog-control-center/src/config.rs
  • rog-control-center/src/main.rs
  • rog-control-center/src/ui/mod.rs
  • rog-control-center/src/ui/setup_aura.rs
  • rog-control-center/src/ui/setup_slash.rs
  • rog-control-center/src/ui/setup_system.rs
  • rog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
  • rog-control-center/translations/zh_CN/rog-control-center.po
  • rog-control-center/ui/globals.slint
  • rog-control-center/ui/main_window.slint
  • rog-control-center/ui/pages/about.slint
  • rog-control-center/ui/pages/anime.slint
  • rog-control-center/ui/pages/app_settings.slint
  • rog-control-center/ui/pages/aura.slint
  • rog-control-center/ui/pages/battery.slint
  • rog-control-center/ui/pages/fans.slint
  • rog-control-center/ui/pages/gpu.slint
  • rog-control-center/ui/pages/home.slint
  • rog-control-center/ui/pages/slash.slint
  • rog-control-center/ui/pages/system.slint
  • rog-control-center/ui/types/aura_types.slint
  • rog-control-center/ui/widgets/common.slint
  • rog-control-center/ui/widgets/graph.slint
  • rog-control-center/ui/widgets/rog.slint
  • rog-control-center/ui/widgets/sidebar.slint
  • rog-platform/src/gpu_pci.rs
📜 Review details
🔇 Additional comments (30)
rog-control-center/ui/pages/system.slint (1)

166-166: LGTM!

Also applies to: 177-179

rog-control-center/src/ui/setup_system.rs (1)

90-100: LGTM!

Also applies to: 162-162, 199-199

rog-control-center/src/main.rs (1)

66-85: The reload bypass still races D-Bus name ownership.

This is the same reload handoff race raised previously; it is consolidated with the spawning site in rog-control-center/src/ui/mod.rs.

rog-control-center/src/ui/mod.rs (1)

456-480: The reload bypass still races D-Bus name ownership.

This is the same reload handoff race raised previously; it is consolidated with the guard site in rog-control-center/src/main.rs.

rog-control-center/Cargo.toml (1)

41-41: LGTM!

rog-control-center/src/config.rs (1)

27-28: LGTM!

Also applies to: 45-45, 106-106

rog-control-center/translations/zh_CN/rog-control-center.po (1)

308-312: 🎯 Functional Correctness

Check whether these fuzzy flags are intentional. If these translations are meant to ship, clear the flags and regenerate the catalog so they are included in the compiled .mo.

rog-control-center/ui/types/aura_types.slint (1)

77-78: LGTM!

rog-control-center/src/ui/setup_aura.rs (1)

186-198: LGTM!

rog-control-center/ui/pages/slash.slint (3)

5-109: LGTM!


190-351: LGTM!


597-618: LGTM!

asusd/src/aura_slash/trait_impls.rs (1)

140-144: 🗄️ Data Integrity & Integration

No D-Bus signature change here. SlashMode is byte-backed, so Mode clients reading u8 should still work; the fix is just returning the mode field instead of display_interval.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/globals.slint (2)

1-3: Comments are still in Chinese and point at a local-only file.

The header (and most token comments below) is non-English, and line 3 references ~/ac-rev/AC-逆向设计参考.md, which isn't part of the repo. A previous review asked for English comments; that change hasn't landed here.


5-8: 📐 Maintainability & Code Quality

Confirm IconImages has no remaining consumers. Any page still importing it will fail to compile if the global is gone.

rog-control-center/ui/widgets/rog.slint (1)

10-146: LGTM!

Also applies to: 286-796

rog-control-center/ui/widgets/sidebar.slint (2)

7-58: LGTM!

Also applies to: 60-70, 120-134, 135-166


99-111: 🩺 Stability & Availability

Remove the nav-icon warning
rog-control-center/ui/widgets/sidebar.slint only references ../../data/rog-control-center.png; there are no ../images/nav-*.svg assets here, so this warning doesn’t apply.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/widgets/common.slint (1)

330-330: LGTM!

rog-control-center/ui/widgets/graph.slint (1)

1-9: LGTM!

Also applies to: 50-50, 66-66, 76-76, 92-92, 103-103, 120-132, 141-141, 196-196, 232-232

rog-control-center/ui/main_window.slint (1)

42-43: 🩺 Stability & Availability

Double-check any remaining Rust calls to the removed show_notif / fade_cover setters. Slint no longer generates those setters, so any lingering call sites will break.

rog-control-center/ui/pages/home.slint (2)

216-273: LGTM!


103-104: 📐 Maintainability & Code Quality

Check RogReading.progress clamping. If it does not cap values above 1.0, these hardcoded ceilings can overdraw the bar.

rog-control-center/ui/pages/anime.slint (1)

3-3: LGTM!

Also applies to: 35-35

rog-control-center/ui/pages/app_settings.slint (2)

76-87: 📐 Maintainability & Code Quality

Verify the settings content stays reachable on short windows.

content is height-bounded to the window and the VerticalLayout uses alignment: start with no scroll container. With all toggles plus the shortcut and language cards visible, the lower cards can be clipped (clip: true on the page) with no way to reach them on small window heights. Please confirm the minimum window height accommodates the tallest state (autostart on + shortcut section shown).


90-126: LGTM!

Also applies to: 197-208

rog-control-center/ui/pages/fans.slint (1)

92-149: LGTM!

Also applies to: 156-227

rog-control-center/ui/pages/gpu.slint (2)

109-138: LGTM!


22-25: 🎯 Functional Correctness

Review needs manual confirmation
apu_mem_present/apu_mem_index may still be safe, but the repo search couldn’t resolve the wiring here, so the callback/removal and -1 indexing concerns can’t be settled from the available evidence.

rog-control-center/ui/pages/battery.slint (1)

154-157: 🎯 Functional Correctness

No change needed here. Slint accepts number-to-string concatenation in this expression, so + " W" is fine.

			> Likely an incorrect or invalid review comment.

Comment thread rog-control-center/src/main.rs Outdated
Comment thread rog-control-center/src/ui/mod.rs
Comment thread rog-control-center/ui/pages/about.slint
Comment thread rog-control-center/ui/pages/app_settings.slint
Comment thread rog-control-center/ui/pages/aura.slint
Comment thread rog-control-center/ui/widgets/common.slint
Comment thread rog-control-center/ui/widgets/rog.slint
Comment thread rog-control-center/ui/widgets/rog.slint
Comment thread rog-platform/src/gpu_pci.rs Outdated
Comment thread rog-platform/src/gpu_pci.rs Outdated
@luytan

luytan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@scardracs

Copy link
Copy Markdown
Contributor

Please change all the Chinese comments into English

The Slash mode() getter handed back config.display_interval (a u8) instead
of config.display_mode, so the client always came up on Static.
@NB-Group

Copy link
Copy Markdown
Author

请重写提交记录并以合乎逻辑的方式进行修复。仅仅记录你正在修复我的审查意见,从长远来看并无益处。其他提交记录也一样。

请将所有中文评论替换成英文

Done.

@NB-Group
NB-Group force-pushed the main branch 3 times, most recently from 9c0493a to 62ffd19 Compare July 29, 2026 04:03
Redoes the UI to match Armoury Crate: a self-drawn component library
(rog.slint) on a single Theme global, a dashboard Home page, and AC
styling across System / Aura / Slash / Fan / GPU / Battery / App Settings
/ About. Backend wiring is unchanged — the same Slint global callbacks
drive the same asusd proxies — except for a few things that needed real
data: Home specs and the GPU clock come off live telemetry, the
charge-limit toggle actually reaches asusd (threshold 100 = disabled),
the language picker auto-discovers translations and shows native names,
setlocale runs before the tokio runtime, and the reload-window bypass is
one-shot. Comments are English throughout.
NB-Group added 2 commits July 29, 2026 12:20
Feature list was one @tr with five features joined by newlines, so
translators couldn't reuse them — split into five separate @tr. The
GitHub / asus-linux lines were coloured as links but had no handler;
drop the link colour so they're not mistaken for clickable.
Setting LANG/LC_ALL/LANGUAGE env vars before init_translations is enough:
the reload window already restarts the process, and gettext picks up
LC_ALL to resolve @tr() in the chosen locale on the next start. Removes
the libc::setlocale call, the CString, the unsafe block, and the libc
crate dependency.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

🤖 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 `@rog-control-center/src/ui/mod.rs`:
- Around line 449-458: Update the language-change callback around
config_copy.try_lock so persistence cannot be silently skipped: use a blocking
lock or explicitly handle and retry/report lock failure before logging success.
Ensure the “Language changed” message is emitted only after lock.language is
updated and lock.write() completes successfully.

In `@rog-control-center/translations/zh_CN/rog-control-center.po`:
- Around line 804-807: Update the PageHome translation for the "Turbo" msgid by
removing the accidental leading whitespace from msgstr, preserving the
translated label as “turbo”.
- Around line 308-429: Finalize the fuzzy entries in the translation catalog,
including the dGPU boost label and all “Basic aura mode” entries shown in the
diff, by verifying their Chinese translations and removing their fuzzy markers.
Regenerate the corresponding gettext catalog so these translations are included
in the compiled .mo output.

In `@rog-control-center/ui/globals.slint`:
- Around line 94-97: Replace the duplicated 80/65 °C temperature color ternaries
in the CPU, iGPU, and dGPU temperature bindings in pages/system.slint with calls
to Theme.temp-color(...), passing each existing temperature value and preserving
the current displayed behavior.

In `@rog-control-center/ui/pages/about.slint`:
- Around line 34-37: Add the titlebar-relative height binding to the content
Rectangle in rog-control-center/ui/pages/about.slint lines 34-37 and
rog-control-center/ui/pages/battery.slint lines 37-39, using root.height -
titlebar.height so both page content areas match the existing fans.slint layout.

In `@rog-control-center/ui/pages/app_settings.slint`:
- Around line 203-208: Update the RogDropdownRow language model binding in
AppSettingsPageData so it supplies localized human-readable language names
instead of raw locale codes, while preserving the current_language index and
cb_change_language(index) selection behavior.

In `@rog-control-center/ui/pages/aura.slint`:
- Around line 529-598: Remove the duplicated TUF power-settings modal block and
consolidate the shared New/TUF rendering into a single conditional/component
while preserving both device-type behaviors and toggle callbacks. Prefer
extracting the common modal shell and header/close wiring into a reusable
component in rog.slint, then reuse it for the Old branch where applicable; keep
only device-specific state bindings and aliases at the call sites.
- Around line 89-93: Add mouse-cursor: pointer to the Settings button TouchArea
that toggles show_fade_cover and show_aura_power, matching the sibling Reset
Defaults control. Also add the same cursor property to each of the three modal
close-button TouchAreas.
- Around line 459-468: Add a full-size TouchArea as the first child of each
modal Rectangle guarded by show_aura_power: the New modal, Tuf modal, and Old
modal. Configure each TouchArea to cover its parent and consume pointer events
so clicks on padding or gaps do not reach the underlying fade-cover dismiss
handler.
- Around line 649-655: Populate AuraPageData.supported_power_zones alongside
power_zone_names_old in the legacy-device branch of setup_aura, using the same
pow3r zone data and ordering for both lists. Ensure the selected_zone handler in
AuraPowerGroupOld indexes a populated, matching supported_power_zones list so
legacy selections map safely to the intended zone.

In `@rog-control-center/ui/pages/fans.slint`:
- Around line 133-136: Update the profile-tab flow in FanPageData and the
RogTabBar binding to honor balanced_available, performance_available, and
quiet_available, rather than always exposing all three profiles. Ensure
unavailable profiles cannot be selected and display the existing “not available
on this machine” notice, while preserving the current profile behavior for
available profiles.

In `@rog-control-center/ui/pages/gpu.slint`:
- Around line 52-56: Update mode-icon so unrecognized GPU mode strings do not
return the Hybrid icon; preserve the dedicated and integrated mappings, and use
the same unknown-mode handling convention as mode-title instead of visually
implying Hybrid.
- Around line 140-218: Remove the large commented-out Processes UI markup from
the GPU page. Retain only a concise comment documenting the future backend
requirement and the symbols or setup work needed to restore the feature, such as
process-list population in setup_gpu.rs and the related PageGPU declarations.
- Around line 239-255: Restore an interactive reserved-memory selector in the
GPU page’s “Reserved GPU Memory (iGPU VRAM)” section instead of the static Text
control. Bind its options and selected index to GPUPageData.apu_mem_choices and
GPUPageData.apu_mem_index, and invoke GPUPageData.cb_set_apu_mem(int) when the
user changes the selection.

In `@rog-control-center/ui/widgets/graph.slint`:
- Around line 118-132: Update the pressed state in the graph widget’s states
block so the tooltip remains readable: change tip.background from white to the
existing dark tooltip background, or set the tooltip label’s text color to a
contrasting value for that state. Preserve the current hover behavior and
pressed opacity.

In `@rog-control-center/ui/widgets/sidebar.slint`:
- Line 37: Replace the hardcoded disabled icon color in the sidebar color
expression and the divider background hex values in the relevant divider
elements with the appropriate existing Theme tokens, such as Theme.fg-disabled
and Theme.divider or Theme.line-soft. Keep the enabled and selected color logic
unchanged and reuse the established Theme symbols rather than introducing new
colors.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ca81a82f-b10b-4f21-bee6-7b895a7997b1

📥 Commits

Reviewing files that changed from the base of the PR and between 347048f and e3f6094.

⛔ Files ignored due to path filters (36)
  • rog-control-center/ui/images/effect-breathing.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-cycle.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-flash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-static.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu-fill.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-dedicated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-hybrid.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-integrated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-hz.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-moon.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-speaker.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-sync.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-temp.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-touchpad.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/laptop-placeholder.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-manual.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-performance.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-silent.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-turbo.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-about.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-apps.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-home.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-keyboard.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-matrix.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-slash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-bar.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-device-shell.svg is excluded by !**/*.svg
📒 Files selected for processing (28)
  • asusd/src/aura_slash/trait_impls.rs
  • rog-control-center/Cargo.toml
  • rog-control-center/src/config.rs
  • rog-control-center/src/main.rs
  • rog-control-center/src/ui/mod.rs
  • rog-control-center/src/ui/setup_aura.rs
  • rog-control-center/src/ui/setup_slash.rs
  • rog-control-center/src/ui/setup_system.rs
  • rog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
  • rog-control-center/translations/zh_CN/rog-control-center.po
  • rog-control-center/ui/globals.slint
  • rog-control-center/ui/main_window.slint
  • rog-control-center/ui/pages/about.slint
  • rog-control-center/ui/pages/anime.slint
  • rog-control-center/ui/pages/app_settings.slint
  • rog-control-center/ui/pages/aura.slint
  • rog-control-center/ui/pages/battery.slint
  • rog-control-center/ui/pages/fans.slint
  • rog-control-center/ui/pages/gpu.slint
  • rog-control-center/ui/pages/home.slint
  • rog-control-center/ui/pages/slash.slint
  • rog-control-center/ui/pages/system.slint
  • rog-control-center/ui/types/aura_types.slint
  • rog-control-center/ui/widgets/common.slint
  • rog-control-center/ui/widgets/graph.slint
  • rog-control-center/ui/widgets/rog.slint
  • rog-control-center/ui/widgets/sidebar.slint
  • rog-platform/src/gpu_pci.rs
📜 Review details
🔇 Additional comments (40)
rog-control-center/src/ui/mod.rs (1)

460-484: Make the reload handoff deterministic.

The child bypasses the existing-instance check and can reach D-Bus name registration before the parent releases its connection; the child can then exit on the name-ownership error. This is the previously reported spawn-then-quit race.

rog-control-center/Cargo.toml (1)

41-41: LGTM!

rog-control-center/src/config.rs (1)

27-28: LGTM!

Also applies to: 45-45, 106-106

rog-control-center/src/main.rs (1)

103-120: LGTM!

Also applies to: 200-202, 217-220

rog-control-center/ui/types/aura_types.slint (1)

77-78: LGTM!

rog-control-center/ui/pages/aura.slint (2)

1-19: LGTM!


131-213: LGTM!

rog-control-center/src/ui/setup_aura.rs (1)

186-198: LGTM!

rog-control-center/ui/pages/slash.slint (6)

174-185: 📐 Maintainability & Code Quality | 🏗️ Heavy lift

Hand-rolled slider is still pointer-only — previously flagged, unresolved.

SlashSlider's TouchArea + pointer-event has no FocusScope/keyboard stepping, so brightness and interval remain unreachable for keyboard-only users. Same concern raised in an earlier review round on this exact range.


497-527: 🎯 Functional Correctness | ⚡ Quick win

Mode/interval/brightness controls still gated on SlashPageData.enabled — previously flagged, unresolved.

SlashDevicePreview is designed to animate while lighting is off so users can preview an effect, but the mode ComboBox and both SlashSliders are still enabled: SlashPageData.enabled, so mode/interval/brightness can't be changed while lighting is off — defeating the preview rationale. Same concern raised in an earlier review round on this block.


71-109: 📐 Maintainability & Code Quality | 🏗️ Heavy lift

Custom toggle/click controls are pointer-only — no keyboard or focus support.

SlashSwitch (the primary lighting on/off toggle), the SlashPanelHeader expand/sync hit-areas, and the "Effects/Settings" toggle button are all bare Rectangle + TouchArea with no FocusScope, so keyboard-only users can't enable/disable Slash lighting, expand the panel, or switch to settings. This is the same class of gap already flagged for SlashSlider in this file (see below), just on the switch/button controls instead of the sliders.

🛠️ Suggested direction
-component SlashSwitch inherits Rectangle {
+component SlashSwitch inherits Rectangle {
     in-out property <bool> checked;
     in property <bool> enabled: true;
     callback toggled(bool);
+    forward-focus: focus-scope;
     ...
-    TouchArea {
-        enabled: root.enabled;
-        mouse-cursor: pointer;
-        clicked => {
-            root.checked = !root.checked;
-            root.toggled(root.checked);
-        }
-    }
+    focus-scope := FocusScope {
+        enabled: root.enabled;
+        key-pressed(event) => {
+            if (event.text == " " || event.text == "\n") {
+                root.checked = !root.checked;
+                root.toggled(root.checked);
+                accept
+            } else {
+                reject
+            }
+        }
+        TouchArea {
+            enabled: root.enabled;
+            mouse-cursor: pointer;
+            clicked => {
+                root.checked = !root.checked;
+                root.toggled(root.checked);
+            }
+        }
+    }
 }

The same pattern (wrap in FocusScope, add key handling) should apply to the header hit-areas and the Effects/Settings button.

Also applies to: 54-68, 444-448


1-69: LGTM!


190-351: LGTM!


381-405: LGTM!

Also applies to: 451-467, 530-558

rog-control-center/src/ui/setup_slash.rs (1)

57-90: LGTM!

asusd/src/aura_slash/trait_impls.rs (1)

141-144: 🗄️ Data Integrity & Integration

Mode getter now reads the correct field. It matches set_mode and SlashConfig.display_mode; no further change needed.

rog-control-center/ui/widgets/sidebar.slint (3)

7-58: 🩺 Stability & Availability

Mouse-only nav/quit controls — matches previously deferred accessibility finding.

NavItem and the Quit button only respond to TouchArea, no keyboard focus/activation, the same gap already raised for app_settings.slint/fans.slint and explicitly deferred pending maintainer approval for a broader RogButton accessibility pass.

Also applies to: 145-162


61-134: LGTM!


165-165: 🎯 Functional Correctness

No issue: quit-clicked already exits the app.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/pages/app_settings.slint (2)

172-192: 🩺 Stability & Availability

Mouse-only action rectangles — matches previously deferred accessibility finding.

Same TouchArea-only pattern already flagged (and explicitly deferred pending maintainer approval for a shared RogButton follow-up) for these exact line ranges in a prior review.

Also applies to: 210-229


1-48: LGTM!

Also applies to: 50-170, 194-202

rog-platform/src/gpu_pci.rs (1)

719-770: 🚀 Performance & Scalability

Already-tracked: dGPU identification and repeated NVML init.

This is unchanged from the previously reviewed version: (1) NVML device_by_index(0)/first card*/first amdgpu hwmon fallback should be resolved via the dGPU's actual PCI identity — already agreed as a shared filtering pass across get_gpu_frequency_mhz/get_gpu_temp/get_gpu_usage_pct; (2) Nvml::init() on every call — author noted this matches the existing pattern in this file and offered a follow-up caching pass. Neither is addressed yet in this diff.

rog-control-center/src/ui/setup_system.rs (2)

90-101: DMI product_name read now logs on failure as previously requested.


162-162: LGTM! Correctly wired the same way as the existing cpu_freq_mhz telemetry path (see the system.slint comment for whether the value is actually rendered anywhere).

Also applies to: 199-199

rog-control-center/ui/globals.slint (1)

1-93: LGTM!

rog-control-center/ui/pages/anime.slint (1)

3-3: LGTM!

Also applies to: 35-35

rog-control-center/ui/pages/gpu.slint (1)

1-27: LGTM!

Also applies to: 28-51, 58-138

rog-control-center/ui/pages/system.slint (1)

166-166: 🎯 Functional Correctness

No change needed here gpu_freq_mhz and product_name are already rendered in ui/pages/home.slint, so this isn’t a missing-consumer issue.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/widgets/rog.slint (2)

149-287: 📐 Maintainability & Code Quality

Custom controls still lack focus/keyboard/accessibility support.

RogSlider, RogToggle, RogToggleInt, RogModeSelector, RogTabBar, RogIconCard, and RogEffectButton remain pointer-only (TouchArea with no FocusScope, key-pressed, or accessible-role/accessible-label), unlike the std-widgets controls they replace.


203-237: LGTM!

rog-control-center/ui/widgets/common.slint (2)

5-8: 📐 Maintainability & Code Quality

Hard-coded Theme.bg-card still mixed with Palette.control-foreground children.

RogItem paints a fixed dark background while SystemSlider/SystemToggle*/SystemDropdown text still resolves through Palette, so a light color scheme yields dark-on-dark. Force Palette.color-scheme: dark on the window or move those foregrounds to Theme.text-primary.


330-330: LGTM!

rog-control-center/ui/main_window.slint (2)

92-163: 📐 Maintainability & Code Quality

All nine pages stay instantiated; only painting is skipped.

visible: false avoids drawing but bindings for every page remain live. Already discussed on a previous commit (telemetry polling is gated in setup_system.rs, not per page).


2-4: LGTM!

Also applies to: 26-26, 60-66

rog-control-center/ui/widgets/graph.slint (1)

7-9: LGTM!

Also applies to: 141-141

rog-control-center/ui/pages/home.slint (1)

117-139: LGTM!

Also applies to: 215-272

rog-control-center/ui/pages/battery.slint (2)

76-97: LGTM!


167-170: 🎯 Functional Correctness

No issue here — Slint allows + to concatenate strings and implicitly converts numbers, so this line is fine.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/pages/fans.slint (1)

12-90: LGTM!

Also applies to: 156-227

rog-control-center/ui/pages/about.slint (1)

123-130: 📐 Maintainability & Code Quality

Kernel 6.19+ matches the documented requirement

			> Likely an incorrect or invalid review comment.

Comment thread rog-control-center/src/ui/mod.rs
Comment thread rog-control-center/translations/zh_CN/rog-control-center.po
Comment thread rog-control-center/translations/zh_CN/rog-control-center.po Outdated
Comment thread rog-control-center/ui/globals.slint
Comment thread rog-control-center/ui/pages/about.slint
Comment thread rog-control-center/ui/pages/gpu.slint
Comment thread rog-control-center/ui/pages/gpu.slint
Comment thread rog-control-center/ui/pages/gpu.slint
Comment thread rog-control-center/ui/widgets/graph.slint
Comment thread rog-control-center/ui/widgets/sidebar.slint Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
rog-control-center/src/main.rs (2)

194-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the stale setlocale reference.

These lines claim that setlocale is applied, but the startup code explicitly removed it. Update the comment so it describes the actual locale initialization path.

🤖 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 `@rog-control-center/src/main.rs` around lines 194 - 196, Update the locale
initialization comment near init_translations to remove the stale setlocale
claim and describe only the LANG/LC_ALL environment setup that actually occurs
before Runtime::new. Keep the comment focused on gettext picking up those
environment settings during initialization.

66-75: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Wait for the old D-Bus name to be released before completing reload. In rog-control-center/src/main.rs:125-129, the child still calls Builder::session()?.name(ZBUS_IFACE).build() while the parent keeps the existing connection until shutdown, so the reload handoff can still hit NameTaken. Add a bounded wait/retry or NameOwnerChanged handoff before treating reload as done.

🤖 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 `@rog-control-center/src/main.rs` around lines 66 - 75, The reload handoff must
wait for the parent’s D-Bus name to be released before completing. Update the
child initialization around Builder::session()?.name(ZBUS_IFACE).build() to use
a bounded retry/wait or NameOwnerChanged synchronization, retrying NameTaken
until the old owner exits or the timeout expires, then preserve the existing
failure handling.
🤖 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 `@rog-control-center/src/main.rs`:
- Around line 103-114: Update the startup locale setup before Runtime::new so it
initializes the process locale after setting LANG, LC_ALL, and LANGUAGE, using a
safe setlocale(LC_ALL, "")-equivalent or existing locale initializer. Ensure
this occurs before init_translations! and preserve the configured-language
environment variable behavior.

---

Outside diff comments:
In `@rog-control-center/src/main.rs`:
- Around line 194-196: Update the locale initialization comment near
init_translations to remove the stale setlocale claim and describe only the
LANG/LC_ALL environment setup that actually occurs before Runtime::new. Keep the
comment focused on gettext picking up those environment settings during
initialization.
- Around line 66-75: The reload handoff must wait for the parent’s D-Bus name to
be released before completing. Update the child initialization around
Builder::session()?.name(ZBUS_IFACE).build() to use a bounded retry/wait or
NameOwnerChanged synchronization, retrying NameTaken until the old owner exits
or the timeout expires, then preserve the existing failure handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 06e09109-b51b-490e-93b2-124498a0fbbe

📥 Commits

Reviewing files that changed from the base of the PR and between e3f6094 and 9e173e2.

📒 Files selected for processing (1)
  • rog-control-center/src/main.rs
📜 Review details
🔇 Additional comments (1)
rog-control-center/src/main.rs (1)

211-214: LGTM!

Comment thread rog-control-center/src/main.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

♻️ Duplicate comments (4)
rog-control-center/ui/widgets/graph.slint (1)

120-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Pressed tooltip still renders white text on a white background.

pressed sets tip.background: #FFFFFF`` while label keeps `Theme.text-primary`, so the drag readout is unreadable exactly while dragging. Keep the tooltip dark in `pressed` (or flip the label color for that state).

🎨 Proposed fix
                 pressed when touch.pressed: {
                     point.background: `#FFFFFF`;
-                    tip.background: `#FFFFFF`;
+                    tip.background: Theme.bg-elevated;
                     tip.opacity: 1.0;
                 }
🤖 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 `@rog-control-center/ui/widgets/graph.slint` around lines 120 - 132, Update the
pressed state in the graph widget’s state styling so the tooltip remains
readable during dragging: replace the white `tip.background` used by `pressed`
with the dark tooltip background, or set that state’s label text to a
contrasting color while preserving the existing hover behavior.
rog-control-center/ui/pages/fans.slint (1)

133-136: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Profile tabs still ignore FanPageData.*_available.

quiet_available (default false) and friends are unused, so a Quiet tab remains selectable on machines without that profile and apply pushes Profile.Quiet to the daemon. Either build the tab list from the availability flags or reuse the per-fan "not available on this machine" notice at the profile level.

🤖 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 `@rog-control-center/ui/pages/fans.slint` around lines 133 - 136, The RogTabBar
profile selection in fans.slint must honor FanPageData’s balanced_available,
performance_available, and quiet_available flags. Update the tabs and
selection/apply flow around current-profile so unavailable profiles cannot be
selected or sent to the daemon, while preserving the existing available-profile
behavior and using the established unavailable-profile notice if that is the
existing pattern.
rog-control-center/ui/pages/about.slint (1)

34-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Copied page shell drops the titlebar-relative content height. Both pages replicate the titlebar+content pattern but omit the height binding present in rog-control-center/ui/pages/fans.slint line 116, so each content rect inherits full parent height, is pushed down 52px, and has its bottom padding clipped away.

  • rog-control-center/ui/pages/about.slint#L34-L37: add height: root.height - titlebar.height; to the content Rectangle.
  • rog-control-center/ui/pages/battery.slint#L37-L39: add the same height: root.height - titlebar.height; binding to its content Rectangle.
🤖 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 `@rog-control-center/ui/pages/about.slint` around lines 34 - 37, The content
Rectangle in rog-control-center/ui/pages/about.slint at lines 34-37 must bind
its height to root.height - titlebar.height. Apply the same height binding to
the content Rectangle in rog-control-center/ui/pages/battery.slint at lines
37-39, matching the existing titlebar-relative layout used by the fans page.
rog-control-center/ui/pages/app_settings.slint (1)

203-208: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Language picker still lists raw locale codes.

model: AppSettingsPageData.available_languages surfaces "en"/"zh_CN" rather than "English"/"简体中文". Map discovered codes to display names in the backend while keeping the index contract for cb_change_language.

🤖 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 `@rog-control-center/ui/pages/app_settings.slint` around lines 203 - 208,
Update the backend data source for AppSettingsPageData.available_languages to
map locale codes such as “en” and “zh_CN” to localized display names such as
“English” and “简体中文”, while preserving the list order and index positions used
by current_language and cb_change_language. Keep the RogDropdownRow bindings
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 `@rog-control-center/translations/zh_CN/rog-control-center.po`:
- Around line 283-306: Fill the empty msgstr entries for the five power-limit
labels identified by msgctxt values ppt_pl1_spl, ppt_pl2_sppt, ppt_fppt,
ppt_apu_sppt, and ppt_platform_sppt with accurate Simplified Chinese
translations, preserving the existing gettext structure.

In `@rog-control-center/ui/pages/aura.slint`:
- Around line 355-365: Update the pointer-event handlers in the speed and
brightness TouchAreas to call Math.clamp instead of clamp, and trigger the
existing commit callbacks on both PointerEventKind.up and
PointerEventKind.cancel. Preserve the current live-update behavior for down and
move events.
- Around line 278-280: Replace the second colour block’s `visible:
AuraPageData.colour2_enabled` usage with an `if` conditional around the
`VerticalLayout` so it is removed from layout when disabled. Update
`external_colour_change()` to guard all `c2` accesses because the alias is only
available when that conditional block exists.
- Around line 51-53: Update the content Rectangle in the aura page layout to set
its height to the parent height minus titlebar.height, preserving the existing y
offset so content is constrained to the remaining page area and bottom padding
remains visible.

In `@rog-control-center/ui/pages/home.slint`:
- Around line 215-218: Guard the platform-profile section containing the `for
ev[p] in SystemPageData.platform_profile_indexes` loop with `if
SystemPageData.platform_profile_indexes.length > 0`, including its container and
`Operation Mode` header, so the section collapses entirely when no profiles are
reported and no divisor is evaluated with a zero length.

In `@rog-control-center/ui/pages/slash.slint`:
- Around line 71-109: The custom controls are pointer-only and must support
keyboard activation and visible focus. In
rog-control-center/ui/pages/slash.slint lines 71-109, update SlashSwitch with a
FocusScope that handles Space and Enter by toggling checked and emitting
toggled, and add a clear focus indicator; in lines 431-448, make the
Settings/Effects switcher focusable and activate its existing action from
keyboard input, or replace it with the existing themed button component that
provides focus handling.

In `@rog-control-center/ui/widgets/rog.slint`:
- Line 268: Replace the hardcoded off-state and circle fill colors in RogToggle
and RogEffectButton with the existing Theme.mode-selected and Theme.bg-app
tokens from globals.slint. Preserve the current selected, unselected, checked,
and enabled state behavior while routing each matching value through the
centralized theme definitions.

In `@rog-control-center/ui/widgets/sidebar.slint`:
- Around line 7-58: Update NavItem and the Quit button to be keyboard-accessible
by adding a focusable FocusScope and key handling alongside their existing
TouchArea interactions. Ensure keyboard activation triggers the same clicked
behavior as pointer input, while preserving enabled-state behavior and visual
selection styling.
- Line 64: Update the sidebar logo binding near the is_tuf property so it
selects the TUF-specific logo when is_tuf is true and retains
rog-control-center.png otherwise. Ensure the property is actually used by the
rendered logo, either directly in the sidebar or through the parent window
binding.

---

Duplicate comments:
In `@rog-control-center/ui/pages/about.slint`:
- Around line 34-37: The content Rectangle in
rog-control-center/ui/pages/about.slint at lines 34-37 must bind its height to
root.height - titlebar.height. Apply the same height binding to the content
Rectangle in rog-control-center/ui/pages/battery.slint at lines 37-39, matching
the existing titlebar-relative layout used by the fans page.

In `@rog-control-center/ui/pages/app_settings.slint`:
- Around line 203-208: Update the backend data source for
AppSettingsPageData.available_languages to map locale codes such as “en” and
“zh_CN” to localized display names such as “English” and “简体中文”, while
preserving the list order and index positions used by current_language and
cb_change_language. Keep the RogDropdownRow bindings unchanged.

In `@rog-control-center/ui/pages/fans.slint`:
- Around line 133-136: The RogTabBar profile selection in fans.slint must honor
FanPageData’s balanced_available, performance_available, and quiet_available
flags. Update the tabs and selection/apply flow around current-profile so
unavailable profiles cannot be selected or sent to the daemon, while preserving
the existing available-profile behavior and using the established
unavailable-profile notice if that is the existing pattern.

In `@rog-control-center/ui/widgets/graph.slint`:
- Around line 120-132: Update the pressed state in the graph widget’s state
styling so the tooltip remains readable during dragging: replace the white
`tip.background` used by `pressed` with the dark tooltip background, or set that
state’s label text to a contrasting color while preserving the existing hover
behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ca81a82f-b10b-4f21-bee6-7b895a7997b1

📥 Commits

Reviewing files that changed from the base of the PR and between 5055001 and 9e173e2.

⛔ Files ignored due to path filters (36)
  • rog-control-center/ui/images/effect-breathing.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-cycle.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-flash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/effect-static.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu-fill.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-cpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-dedicated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-hybrid.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu-integrated.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-hz.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-moon.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-speaker.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-sync.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-temp.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-touchpad.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/ic-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/laptop-placeholder.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-manual.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-performance.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-silent.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-turbo.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/mode-windows.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-about.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-apps.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-battery.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-fan.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-gpu.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-home.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-keyboard.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-matrix.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/nav-slash.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-bar.svg is excluded by !**/*.svg
  • rog-control-center/ui/images/slash-device-shell.svg is excluded by !**/*.svg
📒 Files selected for processing (27)
  • asusd/src/aura_slash/trait_impls.rs
  • rog-control-center/src/config.rs
  • rog-control-center/src/main.rs
  • rog-control-center/src/ui/mod.rs
  • rog-control-center/src/ui/setup_aura.rs
  • rog-control-center/src/ui/setup_slash.rs
  • rog-control-center/src/ui/setup_system.rs
  • rog-control-center/translations/zh_CN/LC_MESSAGES/rog-control-center.mo
  • rog-control-center/translations/zh_CN/rog-control-center.po
  • rog-control-center/ui/globals.slint
  • rog-control-center/ui/main_window.slint
  • rog-control-center/ui/pages/about.slint
  • rog-control-center/ui/pages/anime.slint
  • rog-control-center/ui/pages/app_settings.slint
  • rog-control-center/ui/pages/aura.slint
  • rog-control-center/ui/pages/battery.slint
  • rog-control-center/ui/pages/fans.slint
  • rog-control-center/ui/pages/gpu.slint
  • rog-control-center/ui/pages/home.slint
  • rog-control-center/ui/pages/slash.slint
  • rog-control-center/ui/pages/system.slint
  • rog-control-center/ui/types/aura_types.slint
  • rog-control-center/ui/widgets/common.slint
  • rog-control-center/ui/widgets/graph.slint
  • rog-control-center/ui/widgets/rog.slint
  • rog-control-center/ui/widgets/sidebar.slint
  • rog-platform/src/gpu_pci.rs
📜 Review details
🔇 Additional comments (46)
rog-control-center/src/main.rs (2)

103-114: Initialize the process locale before Slint initializes gettext.

Setting LANG/LC_ALL only updates the environment; without a setlocale(LC_ALL, "")-equivalent, LC_MESSAGES may remain C and gettext can ignore LANGUAGE. The comment at Lines 103-106 is therefore incorrect.

Also applies to: 194-195


66-75: LGTM!

Also applies to: 211-215

rog-control-center/src/ui/mod.rs (3)

449-458: Persist the language before reporting success.

A failed try_lock() drops the selection, but Line 456 still says it changed; reload then uses the previous locale.


460-484: Do not treat spawn-and-quit as race-free.

The child bypasses the single-instance check but can still try to own ZBUS_IFACE before the parent releases it. Wait/retry for the old owner to disappear, or use a handoff that guarantees ordering.


144-144: LGTM!

Also applies to: 206-241, 425-448

rog-control-center/translations/zh_CN/rog-control-center.po (2)

308-429: Finalize fuzzy translations and regenerate the catalog.

Gettext omits these entries from the .mo, leaving the dGPU boost and Aura mode labels untranslated.


804-807: Remove the leading whitespace from Turbo.

The rendered label is " turbo".

rog-control-center/src/config.rs (1)

27-28: LGTM!

Also applies to: 45-45, 106-106

rog-control-center/ui/pages/aura.slint (8)

89-93: Settings button's TouchArea still lacks mouse-cursor: pointer (and explicit size) unlike the sibling Reset control at Line 109-111. Same omission on the three modal ✕ TouchAreas (Lines 494, 564, 634).


459-468: Modal still has no TouchArea of its own — clicks on its padding/gaps fall through to the fade-cover dismiss handler at Lines 450-456. Applies to the Tuf (529-538) and Old (599-608) modals too.


529-598: The Tuf modal is still a verbatim copy of the New modal (Lines 459-527); collapse into one conditional or extract a shared modal shell.


649-655: supported_power_zones is still unpopulated on the legacy pathsetup_aura.rs Lines 141-147 only set power_zone_names_old for old laptops, so this index maps into the default enum order and can select the wrong zone or go out of bounds.


1-27: LGTM!


96-128: LGTM!


141-211: LGTM!


335-351: 🎯 Functional Correctness

No change needed. Slint treats these divisions as numeric division here, and clamp() is already used consistently elsewhere in the UI.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/types/aura_types.slint (1)

77-78: LGTM!

rog-control-center/src/ui/setup_aura.rs (1)

186-198: LGTM!

rog-control-center/ui/pages/slash.slint (3)

174-185: Hand-rolled slider still lacks focus/keyboard handling. Same concern as the earlier review on this component.


497-527: Mode/interval/brightness controls gated on SlashPageData.enabled while the preview intentionally animates when off. Same concern as the earlier review.


5-69: LGTM!

rog-control-center/src/ui/setup_slash.rs (1)

57-89: LGTM!

asusd/src/aura_slash/trait_impls.rs (1)

140-144: 🗄️ Data Integrity & Integration

No issue here rog_dbus::zbus_slash::SlashProxy::mode already uses SlashMode, and the in-repo consumer matches that type.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/globals.slint (1)

94-97: 📐 Maintainability & Code Quality

Still not adopted in system.slint.

pages/system.slint (lines 267, 315, 378 in this batch) still inlines its own 80/65 °C ternaries with different colors (#ef4444/#eab308/#22c55e) instead of calling Theme.temp-color(...). Centralization is incomplete and the two color sets now diverge.

rog-control-center/ui/widgets/rog.slint (1)

108-146: 📐 Maintainability & Code Quality

Custom controls still pointer-only — no keyboard/focus support.

RogSlider, RogToggle, RogToggleInt, RogModeSelector, RogTabBar, RogIconCard, and RogEffectButton remain built on bare TouchArea with no FocusScope, key handling, or accessible role/label. The newly-added RogDisplayModeCard (lines 742-795) repeats the exact same pattern. These settings remain unreachable via keyboard or screen reader.

Also applies to: 149-287, 573-627, 630-739, 742-795

rog-control-center/ui/widgets/sidebar.slint (1)

37-37: 📐 Maintainability & Code Quality

Still hardcoded — not routed through Theme.

The disabled icon color (#444444, line 37) and both divider backgrounds (#222222, lines 117 and 142) bypass Theme (e.g. Theme.fg-disabled, Theme.divider) that this file already imports.

Also applies to: 117-117, 142-142

rog-control-center/ui/widgets/common.slint (2)

4-11: 📐 Maintainability & Code Quality

Still mixing Theme background with Palette foregrounds.

RogItem's background/border now come from Theme but SystemSlider, SystemToggle*, and SystemDropdown still paint text via Palette.control-foreground, which follows the resolved Slint color scheme. Under a light scheme this becomes dark-on-dark.


330-330: LGTM!

rog-control-center/ui/pages/gpu.slint (4)

52-56: 🎯 Functional Correctness

Unknown modes still default to the Hybrid icon.

mode-title intentionally shows the raw string for unrecognized modes, but mode-icon silently falls back to the Hybrid icon for the same case.


140-218: 📐 Maintainability & Code Quality

Large commented-out "Processes" block (~80 lines) still kept as dead markup.


239-255: 🎯 Functional Correctness

Reserved GPU memory control is still read-only — regression unresolved.

The static Text display remains in place; GPUPageData.cb_set_apu_mem(int) (declared line 25) is still orphaned, so users still cannot change this setting from the UI.


58-138: LGTM!

rog-platform/src/gpu_pci.rs (1)

722-767: 🎯 Functional Correctness

dGPU identity still unverified across all three probes (tracked follow-up).

get_gpu_frequency_mhz still resolves via device_by_index(0), first card* DRM entry, and first amdgpu hwmon entry without confirming any of them belong to the actual dGPU — same as get_gpu_temp/get_gpu_usage_pct. This was already agreed to be addressed as a single shared PCI-ID-filtering pass across all three readers rather than a one-off; that pass doesn't appear in this diff yet. The Hz→MHz conversion for freq1_input (line 761) is correctly applied.

rog-control-center/src/ui/setup_system.rs (1)

90-101: LGTM!

Also applies to: 162-162, 199-199

rog-control-center/ui/pages/system.slint (1)

166-166: 🎯 Functional Correctness

No issue: gpu_freq_mhz and product_name are used on the Home page.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/widgets/graph.slint (1)

7-9: LGTM!

Also applies to: 141-141

rog-control-center/ui/main_window.slint (2)

2-4: LGTM!

Also applies to: 26-26, 66-66, 73-73


94-105: 🩺 Stability & Availability

opacity is already animated on RogPage, so the fade-out guard is valid. PageAura and PageSlash both inherit RogPage, so self.opacity > 0 keeps inactive pages mounted during the crossfade.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/pages/home.slint (2)

5-11: LGTM!

Also applies to: 53-63, 97-115


221-221: 🎯 Functional Correctness

platform_profile is already the selection index. setup_system.rs maps the enum value to platform_profile_indexes and stores the matching position in SystemPageData.platform_profile, while the Slint view compares against p, reads platform_profile_choices[p], and sends ev on click.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/pages/about.slint (1)

4-31: LGTM!

Also applies to: 59-73

rog-control-center/ui/pages/anime.slint (1)

3-3: LGTM!

Also applies to: 35-35

rog-control-center/ui/pages/app_settings.slint (1)

38-48: LGTM!

Also applies to: 90-126, 129-195

rog-control-center/ui/pages/battery.slint (2)

8-34: LGTM!

Also applies to: 76-97, 130-157


167-170: 🎯 Functional Correctness

float + string concatenation is supported here. Slint implicitly converts float to string, so this won’t fail type-checking.

			> Likely an incorrect or invalid review comment.
rog-control-center/ui/pages/fans.slint (1)

12-90: LGTM!

Also applies to: 92-131, 156-227

Comment thread rog-control-center/translations/zh_CN/rog-control-center.po Outdated
Comment thread rog-control-center/ui/pages/aura.slint
Comment thread rog-control-center/ui/pages/aura.slint
Comment thread rog-control-center/ui/pages/aura.slint
Comment thread rog-control-center/ui/pages/home.slint
Comment thread rog-control-center/ui/pages/slash.slint
Comment thread rog-control-center/ui/widgets/rog.slint Outdated
Comment thread rog-control-center/ui/widgets/sidebar.slint
Comment thread rog-control-center/ui/widgets/sidebar.slint
@NB-Group

Copy link
Copy Markdown
Author

Just checking in.any progress on the PR? No rush.

@Ghoul4500

Copy link
Copy Markdown
Member

Just checking in.any progress on the PR? No rush.

I'm waiting for you to address review comments by coderabbit. Then once you're confident in this, I'll have a look at the code together with others and test this internally for a while before adding to a major release

NB-Group added 2 commits July 29, 2026 23:13
Functional:
- gpu: restore the reserved-VRAM dropdown (cb_set_apu_mem was orphaned behind
  a static Text) via RogDropdownRow.
- fans: gate profile tabs on balanced/performance/quiet_available so an
  unsupported profile can't be selected and written.
- aura: add a click-swallowing TouchArea to each power modal so clicking blank
  regions no longer falls through to the fade-cover and dismisses it.
- aura: persist the speed/brightness drag on PointerEventKind.cancel, not only
  on pointer-up.
- home: hide the Operation Mode section when platform_profile_indexes is empty.
- mod: warn when the language-change config lock is busy instead of silently
  reporting success.

Theming:
- sidebar/rog: route leftover hardcoded hex through Theme tokens.
- system: use Theme.temp-color for the CPU/iGPU/dGPU temperature bindings.
- common: move off Palette.* so control text stays readable on the dark cards.

i18n (zh_CN):
- drop the stray leading space in "Turbo".
- translate PL1/PL2/FPPT/SPPT.

Polish:
- about/battery/aura: give the content rect an explicit height.
- aura: mouse-cursor on the Settings + modal-close buttons; merge the duplicate
  Tuf modal into the New branch.
Missed in the earlier power-limit batch (PL1/PL2/FPPT/SPPT were done);
caught by CodeRabbit.
@NB-Group

Copy link
Copy Markdown
Author

I'm waiting for you to respond to the feedback provided by coderabbit. Once you're satisfied with the solution, I'll review the code with others, and test it internally for a while before deciding to incorporate it into the next major release.

Okay, actually I have already reviewed all of Coderabbit's comments thoroughly. I've done everything that can be accomplished within this pr. The remaining issues reported by Coderabbit will be addressed later.

@coderabbitai coderabbitai Bot added the asusd System Daemon / D-Bus label Jul 29, 2026
@coderabbitai coderabbitai Bot added enhancement New feature or request fix Fix a bug or an issue rog-control-center ROG Control Center GUI labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asusd System Daemon / D-Bus enhancement New feature or request fix Fix a bug or an issue rog-control-center ROG Control Center GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants