Show measurement tool in read-only mode #8334
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to the frontend's UI and tool management system. It adds a new constant Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (3)
frontend/javascripts/oxalis/model/reducers/ui_reducer.ts (2)
Line range hint
1-16: Consider renaming the list and removing the TODO comment.
Having a more descriptive name forALLOWED_TOOLS_IN_VIEW_MODEor removing the// TODO_c make prettiercomment could improve clarity and maintainability.Here's a sample diff adjusting the constant name (lines 12-16 shown for context):
-const ALLOWED_TOOLS_IN_VIEW_MODE = [ +const ALLOWED_VIEW_MODE_ANNOTATION_TOOLS = [ AnnotationToolEnum.MOVE, AnnotationToolEnum.AREA_MEASUREMENT, AnnotationToolEnum.LINE_MEASUREMENT, ];
64-68: Enhance user feedback for disallowed tools in view mode.
When a requested tool is not inALLOWED_TOOLS_IN_VIEW_MODEduringVIEWmode, the reducer silently ignores it. Consider providing user feedback (e.g., a tooltip, a toast, or a log entry) to clarify why the requested tool remains unavailable.frontend/javascripts/oxalis/view/action_bar_view.tsx (1)
240-240: Remove or clarify the commented-out isReadOnly reference.
SinceisReadOnlyis no longer used, consider deleting or further clarifying this comment to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(3 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(3 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(2 hunks)
🔇 Additional comments (4)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (3)
29-29: Importing the new ControlModeEnum looks good.
No issues found.
851-855: Neat approach for deriving isViewMode.
This selector cleanly identifies whether the component is in view mode, improving readability.
1111-1130: Conditional bounding box tool display.
Hiding the bounding box tool in view mode aligns with the intent to limit editing tools. This is consistent with the approach in the reducer.frontend/javascripts/oxalis/view/action_bar_view.tsx (1)
301-301: Simplify the condition for rendering ToolbarView.
This check is adequately scoped byMODES_PLANE.indexOf(viewMode) > -1. The unconditional approach is straightforward. If you expect more modes in the future, consider extracting this logic into a descriptive utility function for scalability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
15-15: Correct grammar and spelling to retain clarity.Consider applying the following changes for better readability and correctness:
- - Measurement tools are now accesible when viewing datasets outside of an annotation. [#8334](https://github.com/scalableminds/webknossos/pull/8334) + - Measurement tools are now accessible when viewing datasets outside annotation context. [#8334](https://github.com/scalableminds/webknossos/pull/8334)🧰 Tools
🪛 LanguageTool
[style] ~15-~15: This phrase is redundant. Consider using “outside”.
Context: ...are now accesible when viewing datasets outside of an annotation. [#8334](https://github.c...(OUTSIDE_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(2 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/oxalis/view/action_bar_view.tsx
- frontend/javascripts/oxalis/model/reducers/ui_reducer.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~15-~15: This phrase is redundant. Consider using “outside”.
Context: ...are now accesible when viewing datasets outside of an annotation. [#8334](https://github.c...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/oxalis/constants.ts (1)
226-227: Looks good!This new constant is consistent with existing naming conventions and the logic of providing measurement tools plus the MOVE tool in view mode. No issues noted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
15-15: Consider simplifying the phrasing.The phrase "outside of" is redundant. Consider revising to:
-Measurement tools are now accessible when viewing datasets outside of an annotation. +Measurement tools are now accessible when viewing datasets outside an annotation.🧰 Tools
🪛 LanguageTool
[style] ~15-~15: This phrase is redundant. Consider using “outside”.
Context: ...re now accessible when viewing datasets outside of an annotation. [#8334](https://github.c...(OUTSIDE_OF)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(5 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/view/action_bar_view.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~15-~15: This phrase is redundant. Consider using “outside”.
Context: ...re now accessible when viewing datasets outside of an annotation. [#8334](https://github.c...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (8)
CHANGELOG.unreleased.md (1)
15-15: LGTM! The changelog entry is well-placed and accurately describes the feature.The entry follows the Keep a Changelog format and correctly references the PR.
🧰 Tools
🪛 LanguageTool
[style] ~15-~15: This phrase is redundant. Consider using “outside”.
Context: ...re now accessible when viewing datasets outside of an annotation. [#8334](https://github.c...(OUTSIDE_OF)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (7)
29-29: LGTM!The addition of
ControlModeEnumimport is well-placed and aligns with the PR's objective to support view mode functionality.
849-853: LGTM! Props and state management look good.The component signature and state management changes are well-structured:
- The
isReadOnlyprop with a default value follows React best practices- The selectors use optional chaining for safe access to potentially null values
861-861: LGTM! Clear and descriptive flag combination.The
isModificationAllowedflag clearly combines the read-only state with volume modification permissions.
Line range hint
946-963: LGTM! Consistent conditional rendering for skeleton tools.The skeleton tools visibility is properly controlled by the
isReadOnlyflag.
1110-1129: LGTM! Proper view mode handling for bounding box tool.The bounding box tool visibility is correctly controlled by the
isReadOnlyflag.
1131-1131: LGTM! Consistent proofread tool visibility control.The proofread tool visibility is properly controlled by combining
isReadOnlywith other required conditions.
Line range hint
1166-1180: Implementation aligns with PR objectives.The measurement tool is correctly implemented to be available in view mode:
- Not restricted by the
isReadOnlyflag- Maintains full functionality while other tools are properly restricted
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/model/reducers/ui_reducer.ts (2)
59-61: Consider handling invalid tools more gracefully.The current implementation silently maintains the existing state when an invalid tool is selected in view mode. Consider providing feedback to the user about why certain tools are not available.
if (AvailableToolsInViewMode.includes(AnnotationToolEnum[action.tool])) { return setToolReducer(state, action.tool); +} else { + // Consider dispatching a notification action here + console.warn(`Tool ${action.tool} is not available in view mode`); }
58-63: Implementation aligns well with PR objectives.The changes effectively enable the measurement tool in view mode while maintaining proper restrictions:
- The view mode check via
allowUpdateensures tools are properly restricted- The
AvailableToolsInViewModevalidation ensures only approved tools (including measurement) are accessible- The implementation maintains existing behavior for other modes
Consider documenting this behavior in the component's documentation to help other developers understand which tools are available in view mode and why.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/oxalis/model/reducers/ui_reducer.ts (1)
1-1: Verify the imported constant's contents.The new imports look good, but let's verify the contents of
AvailableToolsInViewModeto ensure it contains the correct tools for view mode.✅ Verification successful
The
AvailableToolsInViewModeconstant is properly defined and usedThe constant correctly combines measurement tools (LINE_MEASUREMENT, AREA_MEASUREMENT) with the MOVE tool, which is appropriate for view mode functionality while restricting access to editing tools.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the definition and usage of AvailableToolsInViewMode # Find the definition echo "=== AvailableToolsInViewMode definition ===" rg -A 5 "AvailableToolsInViewMode.*=" # Find all usages echo -e "\n=== AvailableToolsInViewMode usages ===" rg "AvailableToolsInViewMode"Length of output: 1149
Script:
#!/bin/bash # Find MeasurementTools definition and related constants echo "=== MeasurementTools definition ===" rg -A 5 "MeasurementTools.*=" echo -e "\n=== AnnotationToolEnum definition ===" rg -A 10 "AnnotationToolEnum.*{"Length of output: 1532
Script:
#!/bin/bash # Find AnnotationToolEnum definition with corrected pattern echo "=== AnnotationToolEnum definition ===" rg -A 10 "enum AnnotationToolEnum"Length of output: 867
|
@MichaelBuessemeyer would you review this? 🙏 Feel free to say no of course. |
knollengewaechs
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.unreleased.md (1)
16-16: Improve clarity of the changelog entry.The phrase "outside of" is redundant. Consider simplifying it.
-Measurement tools are now accessible when viewing datasets outside of an annotation. [#8334](https://github.com/scalableminds/webknossos/pull/8334) +Measurement tools are now accessible when viewing datasets outside an annotation. [#8334](https://github.com/scalableminds/webknossos/pull/8334)🧰 Tools
🪛 LanguageTool
[style] ~16-~16: This phrase is redundant. Consider using “outside”.
Context: ...re now accessible when viewing datasets outside of an annotation. [#8334](https://github.c...(OUTSIDE_OF)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (1)
1112-1131: Consider simplifying the opacity calculation.The opacity calculation could be simplified by using a boolean expression directly.
- opacity: disabledInfosForTools[AnnotationToolEnum.BOUNDING_BOX].isDisabled - ? 0.5 - : 1, + opacity: disabledInfosForTools[AnnotationToolEnum.BOUNDING_BOX].isDisabled ? 0.5 : 1,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/oxalis/constants.ts(1 hunks)frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/constants.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~16-~16: This phrase is redundant. Consider using “outside”.
Context: ...re now accessible when viewing datasets outside of an annotation. [#8334](https://github.c...
(OUTSIDE_OF)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/action-bar/toolbar_view.tsx (3)
851-863: LGTM! Clean implementation of read-only mode.The component properly handles the read-only state and follows React best practices for state management.
Line range hint
948-965: LGTM! Well-implemented skeleton tool visibility control.The implementation properly handles tool visibility based on read-only mode and provides clear user feedback.
1133-1133: LGTM! Measurement tools correctly accessible in read-only mode.The implementation successfully achieves the PR objective by making measurement tools available regardless of read-only state.
| - Added the possibility to configure a rotation for a dataset, which can be toggled off and on when viewing and annotating data. [#8159](https://github.com/scalableminds/webknossos/pull/8159) | ||
| - Added the possibility to use the "merger mode" even when the user has annotated volume data in the current layer (as long as no other mapping is active). [#8335](https://github.com/scalableminds/webknossos/pull/8335) |
There was a problem hiding this comment.
This needs to be removed. Likely a small merging mistake
| - Added the possibility to configure a rotation for a dataset, which can be toggled off and on when viewing and annotating data. [#8159](https://github.com/scalableminds/webknossos/pull/8159) | |
| - Added the possibility to use the "merger mode" even when the user has annotated volume data in the current layer (as long as no other mapping is active). [#8335](https://github.com/scalableminds/webknossos/pull/8335) | |
| - Added the possibility to use the "merger mode" even when the user has annotated volume data in the current layer (as long as no other mapping is active). [#8335](https://github.com/scalableminds/webknossos/pull/8335) |
| const isVolumeModificationAllowed = useSelector( | ||
| (state: OxalisState) => !hasEditableMapping(state), | ||
| ); | ||
| const isModificationAllowed = !isReadOnly && isVolumeModificationAllowed; |
There was a problem hiding this comment.
The code reads to be like the meaning of the new variable isModificationAllowed is still volume related. Therefore, I'd simply keep the name as it was previously and simply the definition here:
| const isVolumeModificationAllowed = useSelector( | |
| (state: OxalisState) => !hasEditableMapping(state), | |
| ); | |
| const isModificationAllowed = !isReadOnly && isVolumeModificationAllowed; | |
| const isVolumeModificationAllowed = !isReadOnly && useSelector( | |
| (state: OxalisState) => !hasEditableMapping(state), | |
| ); |
(Untested; hope this works)
There was a problem hiding this comment.
Oh wait, this might be dangerous. A component should always call the same hooks (including useSelector) in the same order.
To ensure this
| const isVolumeModificationAllowed = useSelector( | |
| (state: OxalisState) => !hasEditableMapping(state), | |
| ); | |
| const isModificationAllowed = !isReadOnly && isVolumeModificationAllowed; | |
| const isVolumeModificationAllowed = useSelector( | |
| (state: OxalisState) => !hasEditableMapping(state), | |
| ) && !isReadOnly; |
should be better
There was a problem hiding this comment.
Awesome. Thanks for developing this and making @valentin-pinkau happy 🎉
The code looks alright to me and also: Very nice that you managed this feature by using only a few line changes 🚤
I only left 2 comments:
- One accidental changelog entry
- Minor naming improvement (needs to be tested whether the suggested code works)
Besides that: Awesome :D
URL of deployed dev instance (used for testing):
Steps to test:
Some screen shots for first impressions:
TODOs:
Issues:
some more context: https://scm.slack.com/archives/C5AKLAV0B/p1737037279255449
(Please delete unneeded items, merge only when none are left open)