Allow to switch to fill or pick segment tool when eraser is active #8314
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the tool control and interaction mechanisms in the frontend. Changes primarily focus on enhancing the behavior of the erase tool and flood fill functionality by adding more nuanced handling of mouse events and keyboard modifier keys. The modifications span multiple files, updating method signatures, adding new methods, and refining the logic for tool adaptations based on user input states like Shift, Control, and Meta keys. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Finishing Touches
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)
frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (1)
353-354: LGTM! Consider adding JSDoc comments.The parameter rename from
isControlPressedtoisControlOrMetaPressedimproves cross-platform compatibility. Consider adding JSDoc comments to document the parameters and their usage./** * Adapts the active tool based on keyboard shortcuts. * @param activeTool - The currently active annotation tool * @param isShiftPressed - Whether the Shift key is pressed * @param isControlOrMetaPressed - Whether the Control key (Windows/Linux) or Command key (Mac) is pressed * @param isAltPressed - Whether the Alt/Option key is pressed * @returns The adapted annotation tool based on the pressed modifier keys */Also applies to: 356-356
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/oxalis/controller/combinations/tool_controls.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/tool_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx(3 hunks)frontend/javascripts/oxalis/view/input_catcher.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (7)
frontend/javascripts/oxalis/model/accessors/tool_accessor.ts (2)
370-380: LGTM! Clear and concise logic for erase tool adaptations.The conditional block for
ERASE_BRUSHandERASE_TRACEtools is well-structured and handles the tool adaptations based on modifier keys appropriately.
392-402: LGTM! Consistent handling of modifier keys.The logic for handling Shift and Control/Meta keys is consistent with the tool adaptation patterns used throughout the codebase.
frontend/javascripts/oxalis/controller/combinations/tool_controls.ts (2)
500-504: LGTM! Improved modifier key handling.The
leftMouseDownmethod now correctly prevents erasing when modifier keys are pressed, allowing for other actions to be triggered instead.
510-519: LGTM! Well-structured tool switching logic.The
leftClickmethod implements a clear hierarchy of tool actions:
- Shift + Control/Meta → Flood fill
- Shift only → Pick cell
frontend/javascripts/oxalis/model/sagas/volume/floodfill_saga.tsx (2)
13-13: LGTM! Added necessary imports.The imports are correctly organized and include the new
getDisabledInfoForToolsfunction.Also applies to: 35-35
121-123: LGTM! Added safety check for disabled tools.The code now correctly checks if the fill cell tool is disabled before proceeding with the flood fill operation.
frontend/javascripts/oxalis/view/input_catcher.tsx (1)
143-143: LGTM! Consistent handling of Control/Meta keys.The changes maintain consistency with the tool accessor updates:
- Variable renamed to reflect both Control and Meta key support
- Function call updated with proper parameter ordering
Also applies to: 151-156
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
20-20: Enhance the changelog entry for clarity.The entry correctly describes the new feature, but could be more explicit about the temporary nature and workflow benefits.
Consider this improved wording:
-When the eraser tool is active, one can switch temporarily to the fill-segment tool by pressing shift and ctrl. Only pressing shift, switches to the pick-segment tool. [#8314](https://github.com/scalableminds/webknossos/pull/8314) +When the eraser tool is active, holding shift+ctrl temporarily switches to the fill-segment tool, while holding shift alone switches to the pick-segment tool. This improves workflow efficiency by reducing tool switching overhead. [#8314](https://github.com/scalableminds/webknossos/pull/8314)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
This contributes to the #8078 as it allows to fill a segment quickly from within the eraser tool (by holding ctrl+shift). Concretely, one can erase cell membranes and then fill one half of the segment with the fill tool while having minimum overhead for tool switching. Alternatively, one would need to move the mouse to the toolbar twice (for switching back and forth) or one would need to use the context menu (which also means two clicks).
To keep things consistent with the other tools, I also added that the shift modifier switches to the pick-segment tool.
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
(Please delete unneeded items, merge only when none are left open)