feat: bus api for homescreen apps and examples #130
Conversation
also removes the mic icon that triggers listen from the app drawer
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
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 (5)
gui/qt5/idle.qml (2)
Line range hint
87-91: LGTM! Consider documenting the visibility behavior.The visibility binding to
appsEnabledis implemented correctly. However, it would be helpful to add a comment explaining when and why the app bar becomes visible/hidden.Add a comment above the AppsBar component:
+ // AppsBar visibility is controlled by the apps_enabled session data + // Shows when apps are available and hides when disabled or no apps present controlBar: Local.AppsBar {
Line range hint
23-28: Consider centralizing session data configuration.The file has multiple session data properties with similar patterns. Consider creating a dedicated SessionConfig component to manage all these configurations in one place, making it easier to maintain and update session-related properties.
Example structure:
// SessionConfig.qml Item { property bool appsEnabled: sessionData.apps_enabled ? Boolean(sessionData.apps_enabled) : false property bool examplesEnabled: sessionData.skill_info_enabled ? Boolean(sessionData.skill_info_enabled) : false // ... other session properties }This would make the code more maintainable and reduce duplication of the Boolean conversion pattern.
__init__.py (3)
168-174: Add error handling for missing keys inmessage.dataIn
handle_register_sample_utterances, the code assumes thatmessage.datacontains the keys"lang","skill_id", and"utterances". If any of these keys are missing, it could raise aKeyError. To improve robustness, consider adding error handling or usingmessage.data.get("key")with default values.You can apply this diff to handle missing keys:
def handle_register_sample_utterances(self, message: Message): + data = message.data + lang = standardize_lang_tag(data.get("lang", self.lang)) + skill_id = data.get("skill_id") + examples = data.get("utterances", []) + if not skill_id or not examples: + LOG.error("Missing skill_id or utterances in message data") + return - lang = standardize_lang_tag(message.data["lang"]) - skill_id = message.data["skill_id"] - examples = message.data["utterances"] if skill_id not in self.skill_examples: self.skill_examples[skill_id] = {} self.skill_examples[skill_id][lang] = examples
183-187: Add error handling for missing keys inmessage.dataIn
handle_register_homescreen_app, the code usesmessage.data["skill_id"],message.data["icon"], andmessage.data["event"]without checking if these keys are present. This could lead to aKeyErrorif any are missing. Consider adding checks or usingmessage.data.get("key").Here's how you might adjust the code:
def handle_register_homescreen_app(self, message: Message): + data = message.data + skill_id = data.get("skill_id") + icon = data.get("icon") + event = data.get("event") + if not skill_id or not icon or not event: + LOG.error("Missing skill_id, icon, or event in message data") + return - skill_id = message.data["skill_id"] - icon = message.data["icon"] - event = message.data["event"] self.homescreen_apps[skill_id] = {"icon": icon, "event": event}
254-254: Rename unused variableskill_idto_skill_idin for loopIn the loop at line 254,
skill_idis assigned but not used in the loop body. To clarify that it's intentionally unused and to prevent warnings from static analysis tools, consider renaming it to_skill_idor_.Apply this diff to rename the unused variable:
def update_examples(self): # ... - for skill_id, data in self.skill_examples.items(): + for _skill_id, data in self.skill_examples.items(): examples += data.get(self.lang, [])🧰 Tools
🪛 Ruff
254-254: Loop control variable
skill_idnot used within loop bodyRename unused
skill_idto_skill_id(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
__init__.py(5 hunks)gui/qt5/AppsBar.qml(0 hunks)gui/qt5/idle.qml(2 hunks)
💤 Files with no reviewable changes (1)
- gui/qt5/AppsBar.qml
🧰 Additional context used
🪛 Ruff
__init__.py
254-254: Loop control variable skill_id not used within loop body
Rename unused skill_id to _skill_id
(B007)
🔇 Additional comments (3)
gui/qt5/idle.qml (1)
23-23: LGTM! Property declaration follows best practices.
The new appsEnabled property is well-defined with proper type safety and fallback handling.
Let's verify the session data handling:
__init__.py (2)
155-163: Event handlers registered correctly
The addition of event handlers for registering and deregistering sample utterances and homescreen apps enhances the functionality as per the PR objectives. The implementation is appropriate and follows the correct pattern.
476-477: LGTM
The build_voice_applications_model method correctly constructs the applications model from self.homescreen_apps. The implementation is clear and aligns with the intended functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
__init__.py (4)
55-59: Add class-level documentation for the new dictionaries.Consider adding docstrings to document the structure and purpose of these dictionaries at the class level for better maintainability.
class OVOSHomescreenSkill(OVOSSkill): + """OVOS Homescreen Skill + + Class Variables: + skill_examples (Dict[str, Dict[str, List[str]]]): Maps skill_id to a dict of + language codes and their corresponding example utterances + homescreen_apps (Dict[str, Dict[str, str]]): Maps skill_id to app metadata + containing icon path and bus event to emit + """
166-193: Add input validation for message data.The handlers should validate required fields to prevent KeyError exceptions and provide meaningful error messages.
Example implementation for the registration handler:
def handle_register_homescreen_app(self, message: Message): """a skill is registering an icon + bus event to show in app drawer (bottom pill button)""" - skill_id = message.data["skill_id"] - icon = message.data["icon"] - event = message.data["event"] + required_fields = ["skill_id", "icon", "event"] + if not all(field in message.data for field in required_fields): + LOG.error(f"Missing required fields: {required_fields}") + return + + skill_id = message.data["skill_id"] + icon = message.data["icon"] + event = message.data["event"] self.homescreen_apps[skill_id] = {"icon": icon, "event": event}
478-479: Add method documentation and return type hint.The method would benefit from clear documentation of its purpose and return type.
def build_voice_applications_model(self): + """Build a list of voice applications for the GUI model. + + Returns: + List[Dict[str, str]]: List of application metadata containing + name, thumbnail path, and action event + """ return [{"name": skill_id, "thumbnail": data["icon"], "action": data["event"]} for skill_id, data in self.homescreen_apps.items()]
256-263: Fix unused loop variable warning.The loop variable
skill_idis not used in the loop body.- for skill_id, data in self.skill_examples.items(): + for _skill_id, data in self.skill_examples.items(): examples += data.get(self.lang, [])🧰 Tools
🪛 Ruff
256-256: Loop control variable
skill_idnot used within loop bodyRename unused
skill_idto_skill_id(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
__init__.py(6 hunks)gui/qt5/idle.qml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gui/qt5/idle.qml
🧰 Additional context used
🪛 Ruff
__init__.py
256-256: Loop control variable skill_id not used within loop body
Rename unused skill_id to _skill_id
(B007)
🔇 Additional comments (1)
__init__.py (1)
155-162: LGTM! Event handlers are well-organized.
The new event handlers follow a consistent naming pattern and are properly registered in the initialize method.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
__init__.py (2)
155-162: Consider using constants for event namesWhile the event registration is correct, consider defining the event names as class constants to prevent typos and make maintenance easier.
+ # Class constants for event names + REGISTER_EXAMPLES_EVENT = "homescreen.register.examples" + DEREGISTER_EXAMPLES_EVENT = "homescreen.deregister.examples" + REGISTER_APP_EVENT = "homescreen.register.app" + DEREGISTER_APP_EVENT = "homescreen.deregister.app" def initialize(self): - self.add_event("homescreen.register.examples", + self.add_event(self.REGISTER_EXAMPLES_EVENT, self.handle_register_sample_utterances) - self.add_event("homescreen.deregister.examples", + self.add_event(self.DEREGISTER_EXAMPLES_EVENT, self.handle_deregister_sample_utterances) - self.add_event("homescreen.register.app", + self.add_event(self.REGISTER_APP_EVENT, self.handle_register_homescreen_app) - self.add_event("homescreen.deregister.app", + self.add_event(self.DEREGISTER_APP_EVENT, self.handle_deregister_homescreen_app)
256-263: Enhance logging for better debuggingConsider adding more detailed logging to help diagnose issues with example registration.
for _skill_id, data in self.skill_examples.items(): + LOG.debug(f"Processing examples for skill {_skill_id}") examples += data.get(self.lang, []) + LOG.debug(f"Total examples count: {len(examples)}") if examples: + LOG.info(f"Updated {len(examples)} examples for language {self.lang}") self.gui['skill_examples'] = {"examples": examples} self.gui['skill_info_enabled'] = self.examples_enabled else: - LOG.warning("no utterance examples registered with homescreen") + LOG.warning(f"No utterance examples registered for language {self.lang}") self.gui['skill_info_enabled'] = False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(6 hunks)
🔇 Additional comments (1)
__init__.py (1)
55-59: LGTM! Well-structured data storage for apps and examples
The new dictionaries with proper type hints provide a clean implementation for managing registered homescreen applications and skill examples.
register utterance examples from skill.json with homescreen register handler with homescreen app drawer companion to OpenVoiceOS/ovos-skill-homescreen#130
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
__init__.py (2)
55-59: Consider adding type hints and docstrings for the new dictionaries.The new dictionaries are well-structured but would benefit from additional documentation.
- # Bus apis for skills to register with homescreen, ovos-workshop provides util methods - # "skill_id": {"lang-code": ["utterance"]} - self.skill_examples: Dict[str, Dict[str, List[str]]] = {} - # "skill_id": {"icon": "xx.png", "event": "emit.this.bus.event"} - self.homescreen_apps: Dict[str, Dict[str, str]] = {} + # Bus APIs for skills to register with homescreen + # + # Dictionary mapping skill IDs to their example utterances in different languages + # Format: {"skill_id": {"lang-code": ["utterance1", "utterance2", ...]}} + self.skill_examples: Dict[str, Dict[str, List[str]]] = {} + + # Dictionary mapping skill IDs to their homescreen app configurations + # Format: {"skill_id": {"icon": "path/to/icon.png", "event": "bus.event.to.emit"}} + self.homescreen_apps: Dict[str, Dict[str, str]] = {}
252-259: Consider adding language fallback for examples.When examples are not available in the current language, consider falling back to a default language.
elif self.settings.get("examples_enabled"): - for _skill_id, data in self.skill_examples.items(): - examples += data.get(self.lang, []) + default_lang = "en-us" # or get from config + for _skill_id, data in self.skill_examples.items(): + # Try current language first, then fall back to default + examples += data.get(self.lang, data.get(default_lang, []))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(6 hunks)
🔇 Additional comments (2)
__init__.py (2)
153-158: LGTM! Event handlers are properly registered.
The event handlers are correctly registered with appropriate event names and handler methods.
182-189: LGTM! Deregistration handler is properly implemented.
The handler correctly removes both examples and app data for the unloaded skill.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(7 hunks)
🔇 Additional comments (2)
__init__.py (2)
55-59: LGTM: Well-structured data storage implementation
The new dictionaries with clear type hints and documentation provide a clean implementation for storing skill examples and homescreen apps.
473-481: 🛠️ Refactor suggestion
Add data validation and error handling
The method should validate the data structure before building the model to prevent runtime errors if the dictionary structure is invalid.
Apply this diff to improve robustness:
def build_voice_applications_model(self) -> List[Dict[str, str]]:
"""Build a list of voice applications for the GUI model.
Returns:
List[Dict[str, str]]: List of application metadata containing
name, thumbnail path, and action event
"""
+ def validate_app_data(skill_id: str, data: Dict[str, str]) -> bool:
+ required_fields = ["icon", "event"]
+ if not all(field in data for field in required_fields):
+ LOG.error(f"Missing required fields for skill {skill_id}: {data}")
+ return False
+ if not os.path.exists(data["icon"]):
+ LOG.warning(f"Icon not found for skill {skill_id}: {data['icon']}")
+ return False
+ return True
+
+ try:
return [{"name": skill_id, "thumbnail": data["icon"], "action": data["event"]}
- for skill_id, data in self.homescreen_apps.items()]
+ for skill_id, data in self.homescreen_apps.items()
+ if validate_app_data(skill_id, data)]
+ except Exception as e:
+ LOG.error(f"Error building applications model: {e}")
+ return []Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
__init__.py (1)
473-481: Add validation for application model dataWhile the implementation is clean, consider adding validation to ensure the required fields exist and are valid before building the model.
def build_voice_applications_model(self) -> List[Dict[str, str]]: """Build a list of voice applications for the GUI model. Returns: List[Dict[str, str]]: List of application metadata containing name, thumbnail path, and action event """ + def validate_app_data(skill_id: str, data: Dict[str, str]) -> bool: + required_fields = ["icon", "event"] + if not all(field in data for field in required_fields): + LOG.error(f"Missing required fields for skill {skill_id}: {data}") + return False + if not os.path.exists(data["icon"]): + LOG.warning(f"Icon not found for skill {skill_id}: {data['icon']}") + return False + return True + return [{"name": skill_id, "thumbnail": data["icon"], "action": data["event"]} - for skill_id, data in self.homescreen_apps.items()] + for skill_id, data in self.homescreen_apps.items() + if validate_app_data(skill_id, data)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
__init__.py(8 hunks)
🔇 Additional comments (3)
__init__.py (3)
55-60: LGTM! Well-structured dictionary definitions.
The new class variables are well-documented and properly typed, making the code more maintainable and self-documenting.
252-259: LGTM! Good error handling.
The update to examples handling is well-implemented with appropriate warning logs when no examples are found.
166-189:
Add comprehensive error handling and validation
The bus API methods need error handling and validation to prevent runtime errors:
- Validate required fields
- Validate data types
- Add error logging
Apply this diff to improve error handling:
def handle_register_sample_utterances(self, message: Message):
"""a skill is registering utterance examples to render on idle screen"""
+ try:
+ required_fields = ["lang", "skill_id", "utterances"]
+ if not all(field in message.data for field in required_fields):
+ LOG.error(f"Missing required fields in message data: {message.data}")
+ return
+
+ if not isinstance(message.data["utterances"], list):
+ LOG.error(f"Invalid utterances format: {message.data['utterances']}")
+ return
+
lang = standardize_lang_tag(message.data["lang"])
skill_id = message.data["skill_id"]
examples = message.data["utterances"]
if skill_id not in self.skill_examples:
self.skill_examples[skill_id] = {}
self.skill_examples[skill_id][lang] = examples
+ except Exception as e:
+ LOG.error(f"Error registering utterances: {e}")
def handle_register_homescreen_app(self, message: Message):
"""a skill is registering an icon + bus event to show in app drawer (bottom pill button)"""
+ try:
+ required_fields = ["skill_id", "icon", "event"]
+ if not all(field in message.data for field in required_fields):
+ LOG.error(f"Missing required fields in message data: {message.data}")
+ return
+
+ if not os.path.exists(message.data["icon"]):
+ LOG.error(f"Icon file not found: {message.data['icon']}")
+ return
+
skill_id = message.data["skill_id"]
icon = message.data["icon"]
event = message.data["event"]
self.homescreen_apps[skill_id] = {"icon": icon, "event": event}
+ except Exception as e:
+ LOG.error(f"Error registering homescreen app: {e}")Likely invalid or redundant comment.
also removes the mic icon that triggers listen from the app drawer
closes #110 and #128
a companion PR in ovos-workshop will be needed to expose this new bus api under util methods and relevant skills updated.
skill examples will be registered automatically from the skill.json on skill load, the application launchers will have a new decorator, OCP, HomeAssistant (PHAL plugin) and local media skill will need to be updated to populate the app drawer with the new decorator
moves default wallpapers to the wallpaper plugin (needs OpenVoiceOS/ovos-PHAL-plugin-wallpaper-manager#22)
Summary by CodeRabbit
Release Notes
New Features
User Interface Changes
Bug Fixes
Dependency Updates
ovos-workshoppackage to>=2.4.0,<3.0.0.These updates enhance the overall user experience by streamlining skill management and adjusting the UI for clarity.