Fix non-default wallpaper handling #24
Conversation
Fix path error in wallpaper collection
WalkthroughThe pull request introduces improvements to the wallpaper management plugin, focusing on type hinting, wallpaper collection handling, and error robustness. The changes enhance the Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (1)
ovos_PHAL_plugin_wallpaper_manager/__init__.py (1)
306-312: Consider improving unknown wallpaper handling.When the current wallpaper isn't found in the collection, setting
current_idx = final_idxmeans the next change will always start from the beginning. This might be confusing for users as it creates an unexpected jump in the rotation sequence.Consider this alternative approach:
- current_idx = final_idx + current_idx = -1 # Start from beginning on next changeThis makes the behavior more intuitive as it follows the natural sequence of wallpapers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_PHAL_plugin_wallpaper_manager/__init__.py(4 hunks)
🔇 Additional comments (4)
ovos_PHAL_plugin_wallpaper_manager/__init__.py (4)
4-4: LGTM! Type hinting import addition.The addition of
Optionalto the typing imports properly supports the type hints added to theget_wallpaper_idxmethod.
80-92: Great security improvements in wallpaper copying logic!The changes effectively prevent overwriting existing wallpapers and add protection against path traversal attacks:
- Path validation before copying
- Existence check before copying
- Improved debug logging
This aligns perfectly with the PR objective of preventing default wallpapers from overwriting existing ones.
96-99: Improved wallpaper collection handling!The collection now properly includes all valid wallpapers from the storage directory, ensuring that both default and non-default wallpapers are loaded correctly. This directly addresses the PR objective of fixing non-default wallpaper handling.
Line range hint
290-295: LGTM! Clear return type specification.The addition of
Optional[int]return type hint improves code clarity and IDE support.
# Description Update wallpaper plugin to resolve observed issue where default wallpaper is missing from options # Issues - Implements OpenVoiceOS/ovos-PHAL-plugin-wallpaper-manager#24 - Needs OpenVoiceOS/ovos-PHAL-plugin-wallpaper-manager#26 # Other Notes <!-- Note any breaking changes, WIP changes, requests for input, etc. here -->
#22 introduced a breaking change where non-default wallpaper files were not loaded by this plugin. It also uncovered an existing issue where an unknown wallpaper being set would break rotation. This PR prevents overwriting existing wallpapers with defaults in the event of a name collision and also ensures non-default wallpapers are loaded by the plugin
Summary by CodeRabbit
New Features
Bug Fixes
Refactor