fix(): Crt dfetail list view, use space better.#149
Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 3 minutes and 8 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a layoutProfile system: theme profiles (defaultTile, crtTile) define list/detail geometry, components (BrowseList, BrowseDetailPane, BrowseListDetailView) accept and use layoutProfile, header/status components gained minor layout props, screens select and propagate the appropriate profile based on CRT-native mode, and small script/ui tweaks were applied. ChangesLayout Profile System and Component Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/screens/SystemsScreen.qml (1)
192-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
_tileLayoutintoBrowseListDetailView
BrowseListDetailViewis not receivinglayoutProfile, so CRT list mode can still render internal list/detail geometry with the default profile despite CRT-specific side margins. AddlayoutProfile: systems._tileLayoutnear Line 204.Suggested fix
BrowseListDetailView { id: listCard @@ anchors.bottom: parent.bottom anchors.bottomMargin: Sizing.pctH(8) + layoutProfile: systems._tileLayout model: Browse.SystemsModel currentIndex: systemsGrid.currentIndex🤖 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/screens/SystemsScreen.qml` around lines 192 - 205, BrowseListDetailView (id: listCard) isn’t receiving the CRT layout profile so list/detail geometry ignores CRT-specific margins; add a layoutProfile property set to systems._tileLayout on the BrowseListDetailView (near the existing visible/anchors/currentIndex properties) so it uses the CRT tile layout when rendering.
🤖 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 `@src/ui/components/HeaderBar.qml`:
- Line 55: The width binding for the progress label uses raw geometry
Math.max(0, Math.floor(parent.width / 4)) and must be routed through the Sizing
helper; update the width expression in HeaderBar.qml (the progress label's width
property) to wrap the computed value with Sizing.px(...) so the computed
Math.floor(parent.width / 4) is passed into Sizing.px and preserves the Math.max
check.
In `@src/ui/theme/BrowseLayouts.qml`:
- Around line 95-126: The listed hardcoded geometry properties (e.g.
listCardSideMargin, listDividerOffsetX, listStripHeight, listStripSlotMargin,
listCardPaddingLeft/Right/Top/Bottom, listRowHeight, listCenterSlot,
listScrollbarGap, listSelectionAccentWidth,
detailMetadataYOffset/ExtraHeight/LeftInset/RightInset, detailPanePadding*,
detailImageXOffset/LeftInset/RightInset/ExtraWidth/ExtraHeight/BottomGap,
detailTagRowHeight/Spacing, listRowTextLeftPadding/RightPadding,
listFavoriteRightPadding) must be replaced with Sizing helpers so they scale
correctly; update numeric literals to calls like Sizing.px(value) for pixel
dimensions, Sizing.stroke(...) for border/line widths, Sizing.center(...) for
center-slot semantics (replace listCenterSlot), and Sizing.half(...) where
half-pixel rounding is required, ensuring negative offsets (listDividerOffsetX,
detailMetadataYOffset) use Sizing.px(-N). Keep the same property names but
return Sizing.* values so all geometry-driving values comply with the Sizing
rules.
---
Outside diff comments:
In `@src/ui/screens/SystemsScreen.qml`:
- Around line 192-205: BrowseListDetailView (id: listCard) isn’t receiving the
CRT layout profile so list/detail geometry ignores CRT-specific margins; add a
layoutProfile property set to systems._tileLayout on the BrowseListDetailView
(near the existing visible/anchors/currentIndex properties) so it uses the CRT
tile layout when rendering.
🪄 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
Run ID: 79dd7746-a7f7-48b8-adbb-b29f728bb13e
⛔ Files ignored due to path filters (1)
resources/images/logo-crt.pngis excluded by!**/*.png
📒 Files selected for processing (13)
rust/launcher/.qmlls.inisrc/ui/app/MainLayout.qmlsrc/ui/components/BrowseDetailPane.qmlsrc/ui/components/BrowseList.qmlsrc/ui/components/BrowseListDetailView.qmlsrc/ui/components/HeaderBar.qmlsrc/ui/components/PagedGrid.qmlsrc/ui/components/TopStatusStrip.qmlsrc/ui/screens/FavoritesScreen.qmlsrc/ui/screens/MediaListScreen.qmlsrc/ui/screens/RecentsScreen.qmlsrc/ui/screens/SystemsScreen.qmlsrc/ui/theme/BrowseLayouts.qml
| anchors.left: logo.right | ||
| anchors.leftMargin: Sizing.pctW(1) | ||
| anchors.verticalCenter: logo.verticalCenter | ||
| width: Math.max(0, Math.floor(parent.width / 4)) |
There was a problem hiding this comment.
Route the progress label width through Sizing.px().
The new width binding directly drives geometry without a Sizing wrapper.
As per coding guidelines, "Any value that drives x/y/width/height... must go through Sizing.px(), Sizing.stroke(), Sizing.center(), or Sizing.half()."
🤖 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/components/HeaderBar.qml` at line 55, The width binding for the
progress label uses raw geometry Math.max(0, Math.floor(parent.width / 4)) and
must be routed through the Sizing helper; update the width expression in
HeaderBar.qml (the progress label's width property) to wrap the computed value
with Sizing.px(...) so the computed Math.floor(parent.width / 4) is passed into
Sizing.px and preserves the Math.max check.
asturur
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/screens/SystemsScreen.qml (1)
192-220:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
layoutProfileproperty on BrowseListDetailView.Based on the review stack context, layer 3 added the
layoutProfileproperty toBrowseListDetailView, and layer 5 (current) should propagate the appropriate profile. However, thelayoutProfileproperty is not set on thelistCardinstance.According to the context snippet from
MediaListScreen.qml:286-320,BrowseListDetailViewshould receive:layoutProfile: root._listLayoutProfileIn
SystemsScreen, this should be:layoutProfile: systems._tileLayoutWithout this property,
BrowseListDetailViewcannot properly configure its internalBrowseListandBrowseDetailPanecomponents with the correct layout geometry.🔧 Proposed fix
BrowseListDetailView { id: listCard visible: !systems.transitioning && systems._listLayout anchors.left: parent.left anchors.leftMargin: systems._tileLayout.listCardSideMargin anchors.right: parent.right anchors.rightMargin: systems._tileLayout.listCardSideMargin anchors.top: topStrip.bottom anchors.topMargin: Sizing.pctH(2) anchors.bottom: parent.bottom anchors.bottomMargin: Sizing.pctH(8) + layoutProfile: systems._tileLayout model: Browse.SystemsModel currentIndex: systemsGrid.currentIndex detailTitle: listCard.currentName🤖 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/screens/SystemsScreen.qml` around lines 192 - 220, The BrowseListDetailView instance (id: listCard) is missing the new layoutProfile property; add layoutProfile and set it to systems._tileLayout so BrowseListDetailView (and its internal BrowseList/BrowseDetailPane) receive the correct geometry. Locate the BrowseListDetailView block in SystemsScreen.qml and add layoutProfile: systems._tileLayout to the component (keeping existing ids and handlers like currentIndex, detailTitle, detailCoverKey unchanged).
🧹 Nitpick comments (1)
scripts/run-macos-mister-core.sh (1)
20-21: ⚡ Quick winRemove the commented duplicate line.
Line 21 is a commented duplicate of line 20 and serves no purpose. Remove it to improve clarity.
♻️ Proposed fix
exec "${FRONTEND}" --crt -# exec "${FRONTEND}" --crt🤖 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 `@scripts/run-macos-mister-core.sh` around lines 20 - 21, The script contains a redundant commented duplicate of the exec command: remove the commented line "# exec \"${FRONTEND}\" --crt" so only the active exec "${FRONTEND}" --crt remains; locate the duplicate in the script near the existing exec "${FRONTEND}" --crt line and delete the commented version to improve clarity.
🤖 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.
Outside diff comments:
In `@src/ui/screens/SystemsScreen.qml`:
- Around line 192-220: The BrowseListDetailView instance (id: listCard) is
missing the new layoutProfile property; add layoutProfile and set it to
systems._tileLayout so BrowseListDetailView (and its internal
BrowseList/BrowseDetailPane) receive the correct geometry. Locate the
BrowseListDetailView block in SystemsScreen.qml and add layoutProfile:
systems._tileLayout to the component (keeping existing ids and handlers like
currentIndex, detailTitle, detailCoverKey unchanged).
---
Nitpick comments:
In `@scripts/run-macos-mister-core.sh`:
- Around line 20-21: The script contains a redundant commented duplicate of the
exec command: remove the commented line "# exec \"${FRONTEND}\" --crt" so only
the active exec "${FRONTEND}" --crt remains; locate the duplicate in the script
near the existing exec "${FRONTEND}" --crt line and delete the commented version
to improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 088ccc22-3bce-41e0-8ba0-04737c60083f
📒 Files selected for processing (3)
scripts/run-macos-mister-core.shsrc/ui/screens/GamesScreen.qmlsrc/ui/screens/SystemsScreen.qml
1 participant
Summary
Tweaks to CRT tiles
Tweaks to CRT list games only
