-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(ui-next): Adds toggle state for ToolButton and Crosshair example #5914
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
base: master
Are you sure you want to change the base?
Changes from all commits
487d9d2
cbf93a9
66fe808
882ed3b
9dd75d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||
| import { Enums } from '@cornerstonejs/tools'; | ||||||||||||
| import i18n from '@ohif/i18n'; | ||||||||||||
| import { utils as coreUtils } from '@ohif/core'; | ||||||||||||
| import { utils } from '@ohif/ui-next'; | ||||||||||||
| import { ViewportDataOverlayMenuWrapper } from './components/ViewportDataOverlaySettingMenu/ViewportDataOverlayMenuWrapper'; | ||||||||||||
| import { ViewportOrientationMenuWrapper } from './components/ViewportOrientationMenu/ViewportOrientationMenuWrapper'; | ||||||||||||
|
|
@@ -13,6 +14,8 @@ import TrackingStatus from './components/TrackingStatus/TrackingStatus'; | |||||||||||
| import ViewportColorbarsContainer from './components/ViewportColorbar'; | ||||||||||||
| import AdvancedRenderingControls from './components/AdvancedRenderingControls'; | ||||||||||||
|
|
||||||||||||
| const { ModifierKeyCodeToName } = coreUtils; | ||||||||||||
|
|
||||||||||||
| const getDisabledState = (disabledText?: string) => ({ | ||||||||||||
| disabled: true, | ||||||||||||
| disabledText: disabledText ?? i18n.t('Buttons:Not available on the current viewport'), | ||||||||||||
|
|
@@ -429,6 +432,50 @@ export default function getToolbarModule({ servicesManager, extensionManager }: | |||||||||||
| }; | ||||||||||||
| }, | ||||||||||||
| }, | ||||||||||||
| { | ||||||||||||
| name: 'evaluate.cornerstoneTool.crosshairToggle', | ||||||||||||
| evaluate: ({ viewportId, button, disabledText }) => { | ||||||||||||
| const toolGroup = toolGroupService.getToolGroupForViewport(viewportId); | ||||||||||||
| if (!toolGroup) { | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const toolName = toolbarService.getToolNameForButton(button); | ||||||||||||
| if (!toolGroup.hasTool(toolName)) { | ||||||||||||
| return getDisabledState(disabledText); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const currentMode = toolGroup.getToolOptions(toolName).mode; | ||||||||||||
| // Matches the isOn check in toggleCrosshairsToolbar (commandsModule.ts). | ||||||||||||
| // Both must agree on which modes count as "on" — this evaluator uses it to show the | ||||||||||||
| // toggled button state, the command uses it to decide whether to disable or enable. | ||||||||||||
| // If adding Active mode support (e.g. modifier key), update both. | ||||||||||||
| const isOn = currentMode === Enums.ToolModes.Passive || | ||||||||||||
| currentMode === Enums.ToolModes.Enabled; | ||||||||||||
|
Comment on lines
+453
to
+454
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The For consistency with
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The omission of See the PR message above - this implementation scopes the toolbar button to a pure on/off toggle (Passive ↔ Disabled). The crosshair tool is never set to Active through the toolbar. Active mode will be handled separately at the viewport level in @sedghi's upcoming work via a modifier key interaction. The toolbar button intentionally stays in its toggled appearance during that interaction. Any changes to support Active mode would be part of that follow-up work. |
||||||||||||
|
|
||||||||||||
| if (isOn) { | ||||||||||||
| const toolInstance = toolGroup.getToolInstance(toolName); | ||||||||||||
| const jumpOnClick = toolInstance?.configuration?.jumpOnClick; | ||||||||||||
| if (jumpOnClick?.enabled && jumpOnClick.modifierKey != null) { | ||||||||||||
| const modifierName = ModifierKeyCodeToName[jumpOnClick.modifierKey] || 'Modifier'; | ||||||||||||
| return { | ||||||||||||
| disabled: false, | ||||||||||||
| isActive: false, | ||||||||||||
| isToggled: true, | ||||||||||||
| icon: 'tool-crosshair-checked', | ||||||||||||
| tooltip: `Press ${modifierName} + Click to jump`, | ||||||||||||
| }; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return { | ||||||||||||
| disabled: false, | ||||||||||||
| isActive: false, | ||||||||||||
| isToggled: isOn, | ||||||||||||
| icon: isOn ? 'tool-crosshair-checked' : 'tool-crosshair', | ||||||||||||
| }; | ||||||||||||
| }, | ||||||||||||
|
Comment on lines
+436
to
+477
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The Consider extracting this into a shared helper, or at minimum leaving a comment cross-referencing the two sites so they stay in sync.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two checks should stay in sync. I added cross-reference comments in both locations explaining that they must agree on which modes count as "on," and noting that both need updating if |
||||||||||||
| }, | ||||||||||||
| { | ||||||||||||
| name: 'evaluate.action', | ||||||||||||
| evaluate: () => { | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| /** | ||
| * Bidirectional mapping between modifier-key names and their numeric key-code | ||
| * values. These values mirror the `KeyboardBindings` enum from | ||
| * `@cornerstonejs/tools` (packages/tools/src/enums/ToolBindings.ts). | ||
| * | ||
| * @ohif/core cannot depend on @cornerstonejs/tools directly, so the canonical | ||
| * values are duplicated here. If KeyboardBindings ever changes, update this | ||
| * file to match. | ||
| */ | ||
|
|
||
| /** Modifier key-code → display name (e.g. 17 → 'Ctrl') */ | ||
| const ModifierKeyCodeToName: Record<number, string> = { | ||
| 16: 'Shift', | ||
| 17: 'Ctrl', | ||
| 18: 'Alt', | ||
| 91: 'Cmd', | ||
| }; | ||
|
|
||
| /** Lowercase key name → modifier key-code (e.g. 'ctrl' → 17) */ | ||
| const ModifierKeyNameToCode: Record<string, number> = Object.fromEntries( | ||
| Object.entries(ModifierKeyCodeToName).map(([code, name]) => [name.toLowerCase(), Number(code)]) | ||
| ); | ||
| // 'cmd' → 91 but preferences store 'meta' for the Meta/Cmd key | ||
| ModifierKeyNameToCode.meta = ModifierKeyNameToCode.cmd; | ||
|
|
||
| export { ModifierKeyCodeToName, ModifierKeyNameToCode }; |
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.
Activemode excluded fromisOncheck (same as evaluator)This mirrors the omission in the evaluator: if Crosshairs is somehow in
Activemode (keyboard shortcut or another command),isOnwill befalse, and clicking the toggle button will callsetToolPassiveinstead ofsetToolDisabled, leaving crosshairs active without feedback.The same fix applies here:
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.
Same reasoning as above.
Activemode is out of scope for this PR and will be handled in follow-up work.