Add support for the PyHTMX GUI client #73
Conversation
WalkthroughThe pull request introduces modifications to two files in the ovos_gui module. In Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🔭 Outside diff range comments (1)
ovos_gui/bus.py (1)
Protocol documentation needs to be updated with current message structure
Based on the code analysis, I can confirm that:
- The protocol documentation in
protocol.mdis outdated and doesn't reflect the current message structure- The actual implementation in
ovos_gui/namespace.pyshows the message contains:
gui_idat the root levelframeworkfield for new APIqt_versionfor legacy APIThe protocol documentation should be updated to show:
{ "type": "mycroft.gui.connected", "gui_id": "unique_identifier_provided_by_client", "data": { "framework": "qt6|qt5", // new API "qt_version": "5|6" // legacy API } }🔗 Analysis chain
Line range hint
204-214: Document protocol changes and verify message structureThe change in message structure (nested 'data' field) represents a protocol change that should be documented. Additionally, we should verify the message structure usage across the codebase.
Consider adding a comment documenting the expected message structure:
# Message structure for 'mycroft.gui.connected': # { # 'type': 'mycroft.gui.connected', # 'data': { # 'data': { # 'framework': str, # new API # 'qt_version': int # legacy API # } # } # }
Protocol documentation exists but needs to be updated with new message structure
The protocol documentation exists in
protocol.mdbut doesn't reflect the current message structure with the nested 'data' field and framework/qt_version fields. The documentation should be updated to match the actual implementation.
protocol.md: Update themycroft.gui.connectedmessage structure to include:
- The nested 'data' field
- The 'framework' field for new API
- The 'qt_version' field for legacy API
Let me verify the complete message structure in the codebase:
Let me try one more search to find where these messages are constructed:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for message structure usage echo "Checking message structure usage..." rg -A 5 "mycroft\.gui\.connected" # Search for protocol documentation echo "Checking for protocol documentation..." fd -t f "README|PROTOCOL|transportProtocol" -X catLength of output: 4523
Script:
#!/bin/bash # Search for message structure construction echo "Checking message construction..." rg -A 5 "mycroft\.gui\.connected.*data" # Search for message parsing echo "Checking message parsing..." ast-grep --pattern 'parsed_message.data["data"]'Length of output: 232
Script:
#!/bin/bash # Search for message construction in GUI clients echo "Checking GUI client message construction..." rg -l "gui.*connected" --type py | xargs rg -A 10 "emit.*connected" # Search for message handling echo "Checking message handling..." rg -A 10 "handle_client_connected"Length of output: 2406
🧹 Nitpick comments (1)
ovos_gui/page.py (1)
33-36: Remove redundant else blockThe empty
else: passblock is redundant since it's followed byreturn "". This can be simplified.elif framework in ("py-htmx", ): return "py" -else: - pass return ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_gui/bus.py(1 hunks)ovos_gui/page.py(1 hunks)
🔇 Additional comments (1)
ovos_gui/page.py (1)
33-34: Verify framework string constant usage
The framework identifier "py-htmx" should be consistently used across the codebase. Let's verify this.
✅ Verification successful
Let me check the actual framework-related code in these files to ensure consistency.
Framework string usage is consistent and well-handled
The codebase consistently handles framework identifiers, including "py-htmx". The framework handling is implemented with proper fallbacks and clear logic:
- In
page.py, "py-htmx" is correctly used in the file extension mapping - Framework handling includes proper support for:
- "qt5" (default)
- "qt6"
- "py-htmx"
- The codebase has a robust framework detection system that handles both new API (
frameworkparameter) and legacy API (qt_versionparameter)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for framework string usage
echo "Searching for py-htmx framework string usage..."
rg -l "py-htmx"
# Search for framework-related constants
echo "Checking for framework-related constants..."
rg -l "FRAMEWORK|framework.*=.*['\"]"
Length of output: 358
Script:
#!/bin/bash
# Check content of files with framework references
echo "Content of ovos_gui/page.py:"
rg -A 2 -B 2 "framework" ovos_gui/page.py
echo -e "\nContent of ovos_gui/bus.py:"
rg -A 2 -B 2 "framework" ovos_gui/bus.py
echo -e "\nContent of ovos_gui/namespace.py:"
rg -A 2 -B 2 "framework" ovos_gui/namespace.py
echo -e "\nContent of test/unittests/test_bus.py:"
rg -A 2 -B 2 "framework" test/unittests/test_bus.py
Length of output: 6196
ovos_gui/bus.py
Outdated
| @@ -201,7 +201,7 @@ def on_message(self, message: str): | |||
| default_qt_version = \ | |||
| Configuration().get('gui', {}).get('default_qt_version') or 5 | |||
| msg_type = parsed_message.msg_type | |||
| msg_data = parsed_message.data | |||
| msg_data = parsed_message.data['data'] | |||
There was a problem hiding this comment.
This is probably a misunderstanding on my part because of an omission in the protocol documentation. The protocol defines the client announcing message as:
{
"type": "mycroft.gui.connected",
"gui_id": "unique_identifier_provided_by_client"
}but it does not mention whether reporting the framework attribute should be at the root of the message or under the key data (as a payload). By the sake of similarity, I assumed the message would probably look like a session message (or as the page_gained_focus event message), which is treated by the socket handler like this:
Line 194 in 9ff2777
Since I was not sure, I put both options in the announcing message from the client - https://github.com/femelo/pyhtmx-gui-client/blob/20ffc157931709db7573162d479577e07b0da933/src/pyhtmx_gui/ovos_gui_client.py#L48 - but I haven't had time to discuss it with you.
So, which option should I use?
|
to add support for a new GUI we need to also include all templates all the files prefixed with SYSTEM here need an equivalent for the new GUI https://github.com/OpenVoiceOS/ovos-gui/tree/dev/ovos_gui/res/gui/qt5 (the others can be ignores, they are just implementation details of the SYSTEM_XXX qmls) |
This PR adds support for the PyHTMX GUI client.
Summary by CodeRabbit
New Features
Bug Fixes