fix: gui pages #40
fix: gui pages #40
Conversation
closes #39
WalkthroughThe changes in this pull request primarily focus on refactoring the Changes
Assessment against linked issues
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (2)
ovos_PHAL_plugin_system/__init__.py (2)
Line range hint
269-280: Improve error handling in service restartWhile the GUI page handling is improved, the error handling in the service restart could be enhanced:
- Use specific exception types instead of bare except
- Log the actual error details for debugging
- Consider adding a failure notification to the GUI
Consider this improvement:
- try: - restart_service(service, sudo=False, user=True) - except: - try: - restart_service(service, sudo=True, user=False) - except: - LOG.error("No mycroft or ovos service installed") - return False + try: + restart_service(service, sudo=False, user=True) + except Exception as e: + LOG.warning(f"Failed to restart service as user: {e}") + try: + restart_service(service, sudo=True, user=False) + except Exception as e: + LOG.error(f"Failed to restart service as root: {e}") + self.gui["status"] = "Failed" + self.gui["label"] = "Service restart failed" + self.gui.show_page("Status") + return False
Line range hint
206-270: Overall implementation effectively addresses GUI restart issueThe changes successfully address the GUI restart issue (#39) by:
- Removing deprecated '.qml' extension usage
- Simplifying page identification
- Adding proper override parameters for system operation pages
- Implementing consistent page handling across different operations
However, consider implementing the suggested error handling improvements to make the solution more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_PHAL_plugin_system/__init__.py(3 hunks)
🔇 Additional comments (3)
ovos_PHAL_plugin_system/__init__.py (3)
237-238: LGTM: Consistent implementation with reboot handling
The shutdown page display follows the same pattern as reboot, ensuring consistent behavior across system operations.
206-206: LGTM: Simplified GUI page identification
The change from file paths to direct page names ("Status") aligns with the fix for deprecated '.qml' extension usage mentioned in issue #39.
Let's verify the GUI page exists:
Also applies to: 215-215
✅ Verification successful
GUI page "Status" exists and is properly referenced
The verification confirms that:
- The "Status" QML page exists at
ovos_PHAL_plugin_system/ui/Status.qml - The page is a valid QML file with proper Mycroft.Delegate implementation
- Multiple
show_page("Status")calls in__init__.pycorrectly reference this page
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Status page definition in QML files
fd -e qml . | xargs rg "Status.qml|Status\s*{|page:\s*['\"]Status"
Length of output: 214
Script:
#!/bin/bash
# Search for Status page definition in QML files with fixed regex
fd -e qml . | xargs rg -i "Status\.qml|Status\s*\{|page:\s*['\"]Status['\"]"
# Also search for any QML files with "Status" in their name
fd -e qml . | grep -i "status"
# Search Python files for GUI page registrations
rg -i "register.*page.*status"
Length of output: 184
Script:
#!/bin/bash
# Check the content of the Status.qml file
cat ovos_PHAL_plugin_system/ui/Status.qml
# Search for GUI page registration in the Python file
rg "show_page|register_page" ovos_PHAL_plugin_system/__init__.py
Length of output: 2336
222-223: LGTM: Improved reboot page display with proper overrides
The addition of override_animations and override_idle parameters ensures the reboot page remains visible during the restart process.
Let's verify the Reboot page exists:
✅ Verification successful
Reboot page implementation verified and properly structured
The Reboot.qml file exists and is correctly implemented as a Mycroft delegate page with proper UI components showing a "Rebooting" message and loading animation. The use of override_animations and override_idle in the show_page call is appropriate to ensure this critical system message remains visible during the reboot process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Reboot page definition in QML files
fd -e qml . | xargs rg "Reboot.qml|Reboot\s*{|page:\s*['\"]Reboot"
Length of output: 214
Script:
#!/bin/bash
# Search for Reboot page definition in QML files with fixed regex
fd -e qml . | xargs rg -i "Reboot\.qml|page.*['\"]Reboot['\"]"
# Also check for any QML files with Reboot in the name
fd -e qml -g "*[Rr]eboot*"
# Search Python files for references to Reboot page
rg -i "show.*page.*['\"]Reboot['\"]" --type py
Length of output: 291
Script:
#!/bin/bash
# Let's verify the content of the Reboot.qml file to ensure it's a valid page
cat ovos_PHAL_plugin_system/ui/Reboot.qml
Length of output: 1626
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ovos_PHAL_plugin_system/__init__.py (1)
Line range hint
274-281: Enhance error handling and loggingThe current error handling could be improved to:
- Log specific error messages for each failure case
- Notify the user through the GUI when restart fails
Consider this improvement:
try: restart_service(service, sudo=False, user=True) - except: + except Exception as e: + LOG.error(f"Failed to restart service as user: {e}") try: restart_service(service, sudo=True, user=False) - except: - LOG.error("No mycroft or ovos service installed") + except Exception as e: + LOG.error(f"Failed to restart service: {e}") + self.gui["status"] = "Failed" + self.gui["label"] = "Service restart failed" + self.gui.show_page("Status") return False🧰 Tools
🪛 Ruff (0.8.0)
47-47: SyntaxError: missing closing quote in string literal
49-49: SyntaxError: Expected ',', found name
50-50: SyntaxError: Expected ':', found name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_PHAL_plugin_system/__init__.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
ovos_PHAL_plugin_system/__init__.py
47-47: SyntaxError: missing closing quote in string literal
🔇 Additional comments (5)
ovos_PHAL_plugin_system/__init__.py (5)
207-207: LGTM: Improved GUI page handling
The changes correctly implement direct page name references instead of file paths with .qml extensions, addressing the invalid page warning mentioned in issue #39.
Also applies to: 216-216
238-239: LGTM: Consistent GUI handling for shutdown
The changes mirror the improvements made to the reboot page handling, maintaining consistency in the GUI behavior.
263-263: LGTM: Consistent page handling in language configuration
The change follows the same pattern of using direct page names, maintaining consistency across the codebase.
270-271: LGTM: Consistent restart page handling
The changes align with other GUI improvements, using the same pattern for page display with proper overrides.
223-224: LGTM: Enhanced reboot page display
The addition of override_animations and override_idle parameters should ensure the reboot page displays reliably during system restart.
Let's verify the GUI page exists:
✅ Verification successful
GUI reboot page implementation verified
The Reboot page is properly implemented with:
- Correct QML file structure and imports
- Appropriate layout and styling
- Loading animation integration
- Animation file present and accessible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Reboot page exists in the UI directory
fd -t f "Reboot" -e qml
Length of output: 61
Script:
#!/bin/bash
# Let's check the content of the Reboot.qml file to verify it's properly implemented
cat ovos_PHAL_plugin_system/ui/Reboot.qml
Length of output: 1626
Script:
#!/bin/bash
# Let's verify the animation file exists
ls -l ovos_PHAL_plugin_system/ui/animations/loading.json
Length of output: 159
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ovos_PHAL_plugin_system/__init__.py (2)
Line range hint
274-282: Improve error handling in service restartThe current error handling implementation has several issues:
- Uses bare except clauses which can mask important errors
- Lacks detailed error logging
- Inconsistent return values
Consider this improved implementation:
- try: - restart_service(service, sudo=False, user=True) - except: - try: - restart_service(service, sudo=True, user=False) - except: - LOG.error("No mycroft or ovos service installed") - return False + try: + restart_service(service, sudo=False, user=True) + return True + except Exception as e: + LOG.debug(f"Failed to restart service as user: {e}") + try: + restart_service(service, sudo=True, user=False) + return True + except Exception as e: + LOG.error(f"Failed to restart {service}: {e}") + return False
Line range hint
89-90: Maintain consistent string formattingFor consistency with the rest of the codebase, use double quotes for f-strings.
- LOG.warning(f'Got registration request without a `skill_id`: ' - f'{message.data}') + LOG.warning(f"Got registration request without a `skill_id`: " + f"{message.data}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_PHAL_plugin_system/__init__.py(4 hunks)
🔇 Additional comments (3)
ovos_PHAL_plugin_system/__init__.py (3)
207-207: LGTM: GUI page handling improvements
The changes correctly implement direct page name references instead of file paths with '.qml' extensions, which resolves the invalid page warnings mentioned in issue #39. The consistent use of page names across all show_page calls improves code maintainability.
Also applies to: 216-216, 223-223, 238-238, 263-263, 270-270
Line range hint 292-306: LGTM: Comprehensive event handler cleanup
The shutdown method properly removes all registered event handlers and follows good cleanup practices by calling the parent's shutdown method.
Line range hint 270-282: Verify GUI restart functionality
While the code changes look good, let's verify that the GUI restart functionality works as expected.
✅ Verification successful
GUI restart functionality appears to be properly implemented
The verification shows that:
- The Restart.qml file exists and contains a proper implementation with loading animation and status display
- The GUI show_page call correctly references "Restart" without .qml extension
- The restart_service function is called with appropriate fallback mechanism (user/sudo)
- The QML implementation follows best practices with proper imports and layout structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining .qml references in GUI-related code and verify service configuration
# Check for any remaining .qml references in GUI-related code
echo "Checking for .qml references in GUI code..."
rg -l "\.qml" --type py
# Verify service configuration
echo "Checking service configuration..."
fd -t f "ovos.service" /etc/systemd/system/
Length of output: 351
Script:
#!/bin/bash
# Let's verify the GUI restart functionality more thoroughly
# Check GUI-related imports and usage
echo "Checking GUI-related code..."
rg -A 5 "show_page.*Restart"
# Check restart_service implementation and usage
echo "Checking restart service implementation..."
rg -A 10 "def restart_service"
# Look for any GUI page definitions or references
echo "Checking GUI page definitions..."
fd -t f "Restart.qml"
# Check for any other restart-related functionality
echo "Checking other restart references..."
rg "restart_service\("
Length of output: 1150
Script:
#!/bin/bash
# Let's examine the Restart.qml file and check restart_service implementation
# Check the content of Restart.qml
echo "Checking Restart.qml content..."
cat ovos_PHAL_plugin_system/ui/Restart.qml
# Look for restart_service function definition
echo "Looking for restart_service definition..."
rg -B 5 -A 15 "def restart_service" --type py
# Check for any error handling patterns in the plugin
echo "Checking error handling patterns..."
rg "LOG\.error.*restart" --type py
Length of output: 1960
closes #39
Summary by CodeRabbit
New Features
Bug Fixes
skill_id.Refactor