Feature: word api for texter effect #851
Conversation
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent update introduces enhancements to the text manipulation capabilities within the application. Specifically, it adds new tools for adding, deleting, and focusing on words, alongside a new set of tools for word management. These changes aim to improve user interaction with text effects by providing more control over the content and focus of the displayed text. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
bigredfrog
changed the title
for more information, see https://pre-commit.ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- ledfx/api/virtuals_tools.py (2 hunks)
- ledfx/effects/texter2d.py (1 hunks)
Additional Context Used
Additional comments not posted (4)
ledfx/api/virtuals_tools.py (3)
13-13: The addition of new tools to theTOOLSlist aligns with the PR objectives and enhances the functionality of theVirtualsToolsEndpointclass.
202-226: The implementation of the "add_words" tool is correct and aligns with the PR objectives.
228-252: The implementation of the "del_words" tool is correct and follows the PR objectives.ledfx/effects/texter2d.py (1)
189-201: Thefocus_wordsmethod includes some logic for handling word focus but lacks clarity on the actual focusing mechanism. Please ensure that the focusing logic is fully implemented and clear.
ledfx/api/virtuals_tools.py
Outdated
| if tool == "new_words": | ||
| _LOGGER.info(f"new words: {virtual.name} effect is {virtual.active_effect.name}") | ||
| if virtual.active_effect.name != "Texter": | ||
| return await self.invalid_request( | ||
| "Virtual new_words failed, active effect is not texter2d" | ||
| ) | ||
|
|
||
| words = data.get("words") | ||
|
|
||
| if words is None: | ||
| return await self.invalid_request( | ||
| "Required attribute for new_words, words was not provided" | ||
| ) | ||
|
|
||
| if type(words) is not str or len(words) < 1: | ||
| return await self.invalid_request( | ||
| "new_words words parameter must be a string with at least one character" | ||
| ) | ||
|
|
||
| result = virtual.active_effect.add_words(words, True) | ||
|
|
||
| if result is not None: | ||
| return await self.invalid_request( | ||
| f"new_words failed: {result}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
The implementation of the "new_words" tool is correct and follows the PR objectives. However, for consistency, consider aligning the error message when the active effect is not "Texter" with the other tools (e.g., "texter2d" vs "Texter").
- "Virtual new_words failed, active effect is not texter2d"
+ "Virtual new_words failed, active effect is not Texter"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if tool == "new_words": | |
| _LOGGER.info(f"new words: {virtual.name} effect is {virtual.active_effect.name}") | |
| if virtual.active_effect.name != "Texter": | |
| return await self.invalid_request( | |
| "Virtual new_words failed, active effect is not texter2d" | |
| ) | |
| words = data.get("words") | |
| if words is None: | |
| return await self.invalid_request( | |
| "Required attribute for new_words, words was not provided" | |
| ) | |
| if type(words) is not str or len(words) < 1: | |
| return await self.invalid_request( | |
| "new_words words parameter must be a string with at least one character" | |
| ) | |
| result = virtual.active_effect.add_words(words, True) | |
| if result is not None: | |
| return await self.invalid_request( | |
| f"new_words failed: {result}" | |
| ) | |
| if tool == "new_words": | |
| _LOGGER.info(f"new words: {virtual.name} effect is {virtual.active_effect.name}") | |
| if virtual.active_effect.name != "Texter": | |
| return await self.invalid_request( | |
| "Virtual new_words failed, active effect is not Texter" | |
| ) | |
| words = data.get("words") | |
| if words is None: | |
| return await self.invalid_request( | |
| "Required attribute for new_words, words was not provided" | |
| ) | |
| if type(words) is not str or len(words) < 1: | |
| return await self.invalid_request( | |
| "new_words words parameter must be a string with at least one character" | |
| ) | |
| result = virtual.active_effect.add_words(words, True) | |
| if result is not None: | |
| return await self.invalid_request( | |
| f"new_words failed: {result}" | |
| ) | |
ledfx/api/virtuals_tools.py
Outdated
| if tool == "focus_words": | ||
| _LOGGER.info( | ||
| f"focus words: {virtual.name} effect is {virtual.active_effect.name}") | ||
| if virtual.active_effect.name != "Texter": | ||
| return await self.invalid_request( | ||
| "Virtual focus_words failed, active effect is not texter" | ||
| ) | ||
|
|
||
| index = data.get("index") | ||
|
|
||
| if index is None: | ||
| return await self.invalid_request( | ||
| "Required attribute for focus_words, index was not provided" | ||
| ) | ||
|
|
||
| if type(index) is not int or index < 0: | ||
| return await self.invalid_request( | ||
| "focus_words index parameter must be an integer 0 or greater" | ||
| ) | ||
|
|
||
| result = virtual.active_effect.focus_words(index) | ||
|
|
||
| if result is not None: | ||
| return await self.invalid_request( | ||
| f"focus_words failed: {result}" | ||
| ) |
There was a problem hiding this comment.
The implementation of the "focus_words" tool is correct and aligns with the PR objectives. However, for consistency, consider aligning the error message when the active effect is not "Texter" with the other tools.
- "Virtual focus_words failed, active effect is not texter"
+ "Virtual focus_words failed, active effect is not Texter"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if tool == "focus_words": | |
| _LOGGER.info( | |
| f"focus words: {virtual.name} effect is {virtual.active_effect.name}") | |
| if virtual.active_effect.name != "Texter": | |
| return await self.invalid_request( | |
| "Virtual focus_words failed, active effect is not texter" | |
| ) | |
| index = data.get("index") | |
| if index is None: | |
| return await self.invalid_request( | |
| "Required attribute for focus_words, index was not provided" | |
| ) | |
| if type(index) is not int or index < 0: | |
| return await self.invalid_request( | |
| "focus_words index parameter must be an integer 0 or greater" | |
| ) | |
| result = virtual.active_effect.focus_words(index) | |
| if result is not None: | |
| return await self.invalid_request( | |
| f"focus_words failed: {result}" | |
| ) | |
| if tool == "focus_words": | |
| _LOGGER.info( | |
| f"focus words: {virtual.name} effect is {virtual.active_effect.name}") | |
| if virtual.active_effect.name != "Texter": | |
| return await self.invalid_request( | |
| "Virtual focus_words failed, active effect is not Texter" | |
| ) | |
| index = data.get("index") | |
| if index is None: | |
| return await self.invalid_request( | |
| "Required attribute for focus_words, index was not provided" | |
| ) | |
| if type(index) is not int or index < 0: | |
| return await self.invalid_request( | |
| "focus_words index parameter must be an integer 0 or greater" | |
| ) | |
| result = virtual.active_effect.focus_words(index) | |
| if result is not None: | |
| return await self.invalid_request( | |
| f"focus_words failed: {result}" | |
| ) |
ledfx/effects/texter2d.py
Outdated
| def del_words(self, count): | ||
| _LOGGER.info(f"del words: count {count}") | ||
| _LOGGER.info(f"existing wordblocks: {self.sentence.wordcount}") | ||
|
|
There was a problem hiding this comment.
The del_words method currently only logs information and does not implement the functionality to delete words. Please add the necessary logic to handle word deletion as per the PR objectives.
ledfx/effects/texter2d.py
Outdated
| def add_words(self, words, clear=False): | ||
| _LOGGER.info(f"add words: {words}") | ||
| _LOGGER.info(f"clear: {clear}") | ||
| _LOGGER.info(f"existing wordblocks: {self.sentence.wordcount}") |
There was a problem hiding this comment.
The add_words method currently only logs information and does not implement the functionality to add words. Please add the necessary logic to handle word addition as per the PR objectives.
Very much in development / draft
Summary by CodeRabbit