fix:full_width_pages #60
Conversation
WalkthroughThe changes introduce a new property Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
ovos_gui/res/gui/qt5/SYSTEM_TextFrame.qml (2)
Line range hint
8-14: Consider improvements to overlay colors and property definition
- The hardcoded overlay colors could limit theme flexibility.
- The
hasTitleproperty could be simplified.Consider these improvements:
Mycroft.CardDelegate { id: systemTextFrame - skillBackgroundColorOverlay: "#000000" - cardBackgroundOverlayColor: "#000000" + skillBackgroundColorOverlay: Kirigami.Theme.backgroundColor + cardBackgroundOverlayColor: Kirigami.Theme.backgroundColor fillWidth: true - property bool hasTitle: sessionData.title.length > 0 ? true : false + property bool hasTitle: sessionData.title.length > 0
Line range hint
16-17: Remove debugging color from RectangleThe Rectangle has a hardcoded "blue" color which appears to be a debugging artifact.
Apply this change:
contentItem: Rectangle { - color: "blue" + color: "transparent"ovos_gui/res/gui/qt5/SYSTEM_ImageFrame.qml (2)
Line range hint
41-53: Refactor the fillMode handling for better maintainabilityThe current implementation has several issues:
- Uses magic numbers instead of Qt.FillMode enum values
- Contains debug console.log statements
- Has redundant else-if branches
Consider this cleaner implementation:
onFillChanged: { - console.log(fill) - if(fill == "PreserveAspectCrop"){ - systemImageDisplay.fillMode = 2 - } else if (fill == "PreserveAspectFit"){ - console.log("inFit") - systemImageDisplay.fillMode = 1 - } else if (fill == "Stretch"){ - systemImageDisplay.fillMode = 0 - } else { - systemImageDisplay.fillMode = 0 - } + systemImageDisplay.fillMode = { + "PreserveAspectCrop": Image.PreserveAspectCrop, + "PreserveAspectFit": Image.PreserveAspectFit, + "Stretch": Image.Stretch + }[fill] || Image.Stretch }This change:
- Uses proper Qt enum values for clarity
- Removes debug logging
- Simplifies the logic using an object lookup
- Maintains the same default behavior
Line range hint
34-34: Consider documenting the fill propertyThe
fillproperty controls important layout behavior but lacks documentation about its expected values and purpose.Add a documentation comment:
+ // Controls image scaling behavior. Accepted values: "PreserveAspectCrop", "PreserveAspectFit", "Stretch" property var fill: sessionData.fillovos_gui/res/gui/qt5/SYSTEM_AnimatedImageFrame.qml (1)
Line range hint
39-52: Enhance code clarity and remove debug statementsSeveral improvements can be made to the fill mode handling:
- Replace magic numbers with Qt.FillMode enum values
- Remove console.log statements from production code
- Document the default behavior
Apply this diff to improve the code:
onFillChanged: { - console.log(fill) if(fill == "PreserveAspectCrop"){ - systemImageDisplay.fillMode = 2 + systemImageDisplay.fillMode = Image.PreserveAspectCrop } else if (fill == "PreserveAspectFit"){ - console.log("inFit") - systemImageDisplay.fillMode = 1 + systemImageDisplay.fillMode = Image.PreserveAspectFit } else if (fill == "Stretch"){ - systemImageDisplay.fillMode = 0 + systemImageDisplay.fillMode = Image.Stretch } else { - systemImageDisplay.fillMode = 0 + // Default to Stretch mode when fill type is not specified + systemImageDisplay.fillMode = Image.Stretch } }ovos_gui/res/gui/qt5/SYSTEM_UrlFrame.qml (4)
Line range hint
14-16: Fix incorrect URL validation logicThe current condition will always evaluate to true because a value cannot be both undefined AND null simultaneously. This could lead to attempts to load invalid URLs.
Apply this fix:
- if(typeof pageUrl !== "undefined" || typeof pageUrl !== null){ + if(typeof pageUrl !== "undefined" && pageUrl !== null){ webview.url = pageUrl }
Line range hint
65-77: Add error handling for full-screen transitionsThe full-screen handling stores and restores dimensions but lacks error handling. This could lead to undefined behavior if the stored dimensions are invalid.
Consider adding error handling:
onFullScreenRequested: function(request) { if (request.toggleOn) { + try { flickable.storeCWidth = flickable.contentWidth flickable.storeCHeight = flickable.contentHeight flickable.contentWidth = flickable.width flickable.contentHeight = flickable.height + } catch (e) { + console.error("Failed to store dimensions:", e) + request.reject() + return + } } else { + try { flickable.contentWidth = flickable.storeCWidth flickable.contentHeight = flickable.storeCHeight + } catch (e) { + console.error("Failed to restore dimensions:", e) + request.reject() + return + } } request.accept() }
Line range hint
79-90: Add error handling for JavaScript executionThe content height adjustment lacks error handling for JavaScript execution failures.
Add error handling:
onLoadingChanged: { if (loadRequest.status !== WebEngineView.LoadSucceededStatus) { + console.warn("Page load failed:", loadRequest.errorString) return; } flickable.contentHeight = 0; flickable.contentWidth = flickable.width; runJavaScript ( "document.documentElement.scrollHeight;", - function (actualPageHeight) { + function (actualPageHeight, error) { + if (error) { + console.error("Failed to get page height:", error) + return; + } flickable.contentHeight = Math.max ( actualPageHeight, flickable.height); }); }
Line range hint
127-156: Improve popup implementation robustnessThe popup implementation has several areas that could be improved:
- Hardcoded dimensions relative to root
- No cleanup handling
- Limited error handling in popup web view
Consider these improvements:
Popup { id: popuproot modal: true focus: true - width: root.width - Kirigami.Units.largeSpacing * 1.25 - height: root.height - Kirigami.Units.largeSpacing * 1.25 + width: Math.min(root.width * 0.9, Kirigami.Units.gridUnit * 45) + height: Math.min(root.height * 0.9, Kirigami.Units.gridUnit * 35) closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutsideParent anchors.centerIn: parent + onClosed: { + popupwebview.url = "about:blank" + } WebEngineView { id: popupwebview anchors.fill: parent url: "about:blank" // ... existing settings ... onNewViewRequested: function(request) { - console.log(request.destination) + console.warn("Nested popups are not supported:", request.destination) + request.reject() } + + onLoadingChanged: { + if (loadRequest.status === WebEngineView.LoadFailedStatus) { + console.error("Popup load failed:", loadRequest.errorString) + } + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
ovos_gui/res/gui/qt5/SYSTEM_AnimatedImageFrame.qml(1 hunks)ovos_gui/res/gui/qt5/SYSTEM_HtmlFrame.qml(1 hunks)ovos_gui/res/gui/qt5/SYSTEM_ImageFrame.qml(1 hunks)ovos_gui/res/gui/qt5/SYSTEM_TextFrame.qml(1 hunks)ovos_gui/res/gui/qt5/SYSTEM_UrlFrame.qml(1 hunks)
🔇 Additional comments (6)
ovos_gui/res/gui/qt5/SYSTEM_HtmlFrame.qml (1)
11-11: LGTM! The fillWidth property addition aligns with the PR objectives.
The change correctly implements full-width behavior for HTML frames, consistent with the PR's goal of standardizing page display.
Let's verify this change is consistently applied across other similar frame components:
✅ Verification successful
The fillWidth: true property is consistently applied across all SYSTEM frame components
The verification confirms that the fillWidth: true property is properly set in all related SYSTEM frame components:
- SYSTEM_AnimatedImageFrame.qml
- SYSTEM_ImageFrame.qml
- SYSTEM_HtmlFrame.qml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify fillWidth property is consistently applied across all SYSTEM frame components
# Expected: All SYSTEM frame QML files should have the fillWidth property set to true
# Search for all SYSTEM frame QML files and check for fillWidth property
fd -e qml SYSTEM_ -x grep -l "Mycroft.Delegate" {} \; | while read -r file; do
echo "=== Checking $file ==="
grep -A 5 "Mycroft.Delegate {" "$file"
done
Length of output: 1532
ovos_gui/res/gui/qt5/SYSTEM_TextFrame.qml (2)
12-12: LGTM: fillWidth property addition
The addition of fillWidth: true aligns with the PR objectives to ensure consistent full-width page display.
Line range hint 7-42: Verify consistent behavior with other system templates
The implementation looks correct, but let's verify it maintains consistency with other system templates.
✅ Verification successful
Consistent fillWidth implementation confirmed across system templates
The verification shows that all system templates (SYSTEM_*Frame.qml) consistently use fillWidth: true and proper layout management:
- All templates use
fillWidth: trueat the delegate level - Layout components (ColumnLayout, Image, etc.) properly use
Layout.fillWidth: true - Anchor usage is consistent and complementary to width settings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent fillWidth property usage across system templates
# and verify no conflicting width settings
# Search for other system templates and their width-related properties
rg -l "SYSTEM_.*\.qml" | xargs rg -l "fillWidth|width:"
# Check for potential conflicting anchors that might affect fillWidth
rg -l "SYSTEM_.*\.qml" | xargs rg "anchors\.(left|right|horizontalCenter)"
Length of output: 134
Script:
#!/bin/bash
# Let's try a different approach to find system templates and their properties
# First, let's find all SYSTEM QML files
fd "SYSTEM.*\.qml$"
# Then let's examine their content specifically for width-related properties
fd "SYSTEM.*\.qml$" --exec cat {}
Length of output: 14201
ovos_gui/res/gui/qt5/SYSTEM_ImageFrame.qml (1)
13-13: LGTM: fillWidth property addition aligns with PR objectives
The addition of fillWidth: true to the Delegate component ensures consistent full-width display behavior, which directly addresses the PR's goal of fixing full-width page issues.
ovos_gui/res/gui/qt5/SYSTEM_AnimatedImageFrame.qml (1)
13-13: LGTM: fillWidth property addition aligns with PR objectives
The addition of fillWidth: true ensures consistent full-width display behavior, which directly addresses the PR's goal of fixing full-width page display issues.
ovos_gui/res/gui/qt5/SYSTEM_UrlFrame.qml (1)
11-11: LGTM: fillWidth property addition
The addition of fillWidth: true aligns with the PR objectives to ensure consistent full-width page display.
avoid inconsistencies like
example above is from speedtest skill, in a mk2 it works as intended due to screen resolution, in x86 we get the above instead of a page replacing the previous one
by default a single page should be displayed, scenarios where this isnt desired should use their own QML not system templates
Summary by CodeRabbit
Release Notes
New Features
fillWidthproperty across several components to enhance layout responsiveness.SYSTEM_ImageFrame.SYSTEM_UrlFrame.Bug Fixes
onPageUrlChangedhandler for URL validation in theSYSTEM_UrlFrame.