-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(toolbox): Refactor Toolbox State Management and fix it #4825
base: master
Are you sure you want to change the base?
Conversation
…tions - Add dynamic threshold brush tools for circular and spherical shapes - Improve threshold tool configuration and activation logic - Update segmentation toolbar with new threshold tool modes and settings - Modify StudyItem component to use asChild prop for tooltips
This commit makes several changes: - Move Toolbox import from @ohif/ui-next to @ohif/extension-default - Remove Toolbox component from ui-next platform - Clean up import statements across multiple extensions - Remove unused console.debug statement - Simplify some code in core services and tool configurations
This commit introduces a new Toolbox utility component in the default extension, which provides a flexible and generic way to manage and render toolbar tools and their options. The component: - Supports dynamic tool option handling - Integrates with toolbar and commands managers - Allows for complex tool configuration and interaction - Provides a reusable approach to creating custom control panels
This commit improves the toolbar interaction mechanism by: - Modifying onInteraction method to handle more complex command scenarios - Updating ToolbarService to process button and option commands dynamically - Simplifying toolbar-related components by removing redundant logic - Improving type definitions for toolbar buttons and options - Streamlining command execution across different toolbar interactions
This commit enhances the toolbar and tool settings functionality by: - Updating ToolbarService to support more dynamic button and option handling - Modifying option change mechanism with enhanced event broadcasting - Refactoring RowSegmentedControl and ToolSettings components to use onChange pattern - Simplifying ToolboxContext reducer and API - Adding optional component prop to Button type definition
This commit introduces several changes to improve the segmentation mode: - Add a new 'Shapes' toolbar button with Circle, Sphere, and Rectangle scissor tools - Modify segmentation mode toolbar to focus on segmentation-specific tools - Update toolbar service to handle more complex button and option interactions - Add debug point in brush size setting command - Refactor toolbar and toolbox components to support dynamic tool configuration
This commit removes the ToolboxContext and its associated provider from the UI library, cleaning up unused context and simplifying the application's provider structure. Changes include: - Deleting ToolboxContext.tsx file - Removing ToolboxProvider imports and usage in App.tsx - Removing ToolboxProvider and useToolbox exports from ui-next index and context providers
This commit streamlines the toolbar button command processing by: - Removing redundant item and command extraction logic - Simplifying option and command retrieval from button props - Reducing nested object access in toolbar interaction
This commit introduces a new `processCommands` method in ToolbarService to: - Unify command handling logic across toolbar interactions - Simplify command processing in useToolbar hook - Remove redundant command type checking - Enhance event broadcasting with more detailed state information
…nds method This commit further streamlines toolbar command handling by: - Removing the `processCommands` method from ToolbarService - Directly using `commandsManager.run` in both useToolbar and ToolbarService - Standardizing command execution with a consistent options structure - Removing redundant command type checking logic
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Viewers
|
Project |
Viewers
|
Branch Review |
feat/threshold-brush
|
Run status |
|
Run duration | 03m 08s |
Commit |
|
Committer | sedghi |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
2
|
|
0
|
|
43
|
View all changes introduced in this branch ↗︎ |
onInteraction={() => | ||
onInteraction?.({ groupId, itemId: item.id, commands: item.commands }) | ||
} | ||
onInteraction={() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you preserve the event? Otherwise it is difficult for some interactions to popup a localized menu etc.
@@ -1 +1,2 @@ | |||
export { addIcon } from './addIcon'; | |||
export { Toolbox } from './Toolbox'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consistently use export * from './Toolbox'
so that it becomes easy to add new exports. That also means any types defined will get exported.
commandsManager, | ||
title, | ||
...props | ||
}: withAppTypes<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using useSystem() rather than withAppTypes? That separates the services used from the actual parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ToolbarService is going to get modified like this, could we please use it to define the hierarchical toolbars?
That is, items such as "MeasurementTools" item should itself be defined as a button, but also use a nested toolbar to define it's contents with a toolbar called "MeasurementTools"
That is, the mode would have to register the "primary" toolbar but also "More Tools" toolbar, and the content of that would be the id's of the buttons included. That then makes the toolbar button definitions reuseable in multiple places, and allows defining alternate menu setups just be re-using the button definitions. Those button definitions should then all go into the extensions unless there are specific ones needing to be added in the mode that weren't already defined. I'm not asking for that second part to be done, just to make the toolbars actually recursive.
The MeasurementTools button parent class should then use useToolbar
itself to get the list of tools to use at the time it is displayed.
@@ -1,3 +1,2 @@ | |||
import { ToolboxUI } from './ToolboxUI'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use export * from './ToolboxUI'
@@ -96,7 +96,7 @@ function modeFactory({ modeConfiguration }) { | |||
|
|||
toolbarService.addButtons(toolbarButtons); | |||
toolbarService.createButtonSection('primary', [ | |||
'MeasurementTools', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should stay MeasurementTools - what it means is that you want to put the "MeasurementTools" button there, which happens to useToolbar('measurementTools'), but that is an internal detail of the MeasurementTools button.
{ | ||
id: 'MeasurementTools', | ||
id: 'measurementSection', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested, this is still a button, so it uses the same casing as the other buttons.
The id of it is 'MeasurementTools' as a button.
The name of the button section to use is 'measurementSection'
They are different objects and shouldn't use the same names.
this.state.buttonSections[key] = buttons; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should have to know anything about the nested button section - that update should be handled by an entirely different useToolbar, and should get the appropriate enhanced information when the toolbar is actually used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the way to make this order independent is to disconnect the sub items generation/fetch from the ToolbarService. Instead, I think it belongs in ToolButtonListWrapper as a simple useToolbar directive on the contained buttonSection id. That way the id of the toolbar item is completely independent of the actual button section id. In fact, one doesn't even know if it USES a nested list. All the logic for that is contained within the wrapper itself, and it becomes possible to write new wrappers that do other types of behaviours with the included items.
- Add `commands` parameter to toolbar interaction events in ToolbarButtonGroupWithServices and ToolBoxWrapper - Implement `getButtonPropsInButtonSection` method in ToolbarService to retrieve button properties for a section - Update ToolboxUI to use new toolbar service method for finding active tool options
- Update `useSegmentations` hook to use `useSystem` for service retrieval - Enhance Toolbox component with active tool options and dynamic rendering - Refactor TMTV extension modules to simplify component dependencies - Optimize threshold calculation in TMTV extension utilities - Add `.cursor/` to `.gitignore`
- Upgrade Cypress to version 14.1.0 - Update @cypress/request and related dependencies - Modify basic test mode and longitudinal mode toolbar configurations - Add window level presets to toolbar buttons - Temporarily disable a Cypress test for snapshot file download - Update Cypress aliases for toolbar buttons
Issues
ToolbarService
can manage all states itself.Changes:
ToolboxContext
and provider have been removed.Toolbarservice
.Toolbarservice
.Test with this
Cannot Show tool options
Before
CleanShot.2025-03-03.at.23.07.40.mp4
After
CleanShot.2025-03-03.at.23.14.42.mp4
Cannot hydrate active tool options
Before
CleanShot.2025-03-03.at.23.16.02.mp4
After
CleanShot.2025-03-03.at.23.16.35.mp4
Unlock advanced tool options placement anywhere in the UI
example:
Enable Action and Other Types of Buttons Inside the Toolbox
Previously, clicking an icon would activate it. Now, certain icons (like interpolation or segment bidirectional) should only trigger an action.
Migration Guides
ToolBox
Previously, the segmentation toolbox was not using an
evaluator
property. This is now taken into accountevaluators in Toolbox
Replace Toolbox imports from ui-next with extension-default
Ensure you're importing the Toolbox component from the correct location:
Remove ToolboxProvider from composition root
If you have the ToolboxProvider in your application composition, remove it:
// In App.tsx or similar const appComposition = [ [ThemeWrapperNext], [ThemeWrapper], [SystemContextProvider, { commandsManager, extensionManager, hotkeysManager, servicesManager }], - [ToolboxProvider], [ViewportGridProvider, { service: viewportGridService }], // Other providers... ];
we now keep the state for toolbar inside the ToolbarService itself
3. Update tool option handlers to use onChange instead of commands
Tool Definitions: Moving to Section-Based Definitions
This migration represents a significant step toward a more extension-based toolbar system. We've moved away from the nested primary/items structure in favor of a flatter, more composable section-based approach.
Deprecated: Nested Toolbar Structures
New Approach: Section-Based Definitions
and then in your mode you can compose the section and associate buttons
:::note
The
measurementSection
is defined in the tool button configuration of theMeasurementTools
button.:::
Group Evaluators Deprecated
Group evaluator functions like
evaluate.group.promoteToPrimaryIfCornerstoneToolNotActiveInTheList
are now deprecated. Instead, theuiType
component itself is responsible for grouping and displaying the buttons from a section as needed. This allows for more flexible UI implementations that aren't tied to specific evaluation logic.