[Bug] Toolbar and hotkeys bugs

sedghi commented 8 months ago

Describe the Bug

There seem to be various bugs that are related to the toolbar, and hotkeys.

Basically the requirements are

There should be 3 types of tools/icons that we support in the toolbar: Toggles, Actions, Tools

There are bugs regarding toggles states and nested menu behaviour which I will explain next.

Toggles

Toggles (they have on and off state e.g., imageSync, referenceLines, CINE player etc.)
should be able to run by ON or OFF state by default, e.g., should be able too choose to run the viewer with references lines on (issue reported here) and here)
They can live on the toolbar as primary or secondary (nested)
Upon user interaction (either mouse/touch click or hotkeys) they should show a different ON state color (as we do have for reference lines for instance)

CleanShot 2024-02-11 at 22 56 25

Current Bugs for Toggles:

Go to here
On the right most menu, click on the dropdown
Click on a toggle (e.g., reference lines, CINE player works fine for some reason)
see the window level tool is lost and the user cannot window level by mouse primary
Correct behaviour: The reference line toggle should not affect the mouse primary as it is only toggle of an state

pretty sure this bug is in setToolActive in the commandsModule

Fixed in https://github.com/OHIF/Viewers/pull/3961

Nested Menu wrong behaviour for actions

Select rectangle on the right most menu (nested)
Then select the rotate right
See now the rotate right is highlighted
Correct behaviour: should not be highlighted, only put on the top, highlight light blue should be associated with only active tool

Fixed in https://github.com/OHIF/Viewers/pull/3961

Nested Menu wrong behaviour for toggles

Select rectangle on the right most menu (nested)
create 1x2 layout
Then select reference line tool
See it moves the reference lines to the top with correct color (dark blue)
click on the reference lines (which is now in top) again
see the color does not change but the functionality becomes toggled
If you do the same thing with image overlay, it should get turned off and become the top (since its default is on), but it wrongfully stays as ON as dark blue highlight

Fixed in https://github.com/OHIF/Viewers/pull/3961

Hotkeys

Hotkeys are not connected to the toolbar

Easy case

Hit Z on the keyboard
see zoom is now on mouse primary
see the zoom icon is not set properly and still window level is highlighted

Harder case

Hit R on the keybaord
see the image rotates
the toolbar does not change any state
Correct behaviour: Should detect there is a rotate right on the last nested icons, and bring that to the top, ideally it should flashes/highlights that as a user interaction too

What would happen if there is a active tool in the nested menu, and the hotkey for another nested icon is clicked?

Again I think we should priotize the user intend which is the lastes

Minor Details

Dropdowns should not hide the primary tool

CleanShot 2024-02-11 at 23 01 06

If you click on any item on the dropdown it correctly moves to the top but it get disappeared from the dropdown for some reason which is weird.

Fixed in https://github.com/OHIF/Viewers/pull/3961

Steps to Reproduce

s

The current behavior

s

The expected behavior

Toggle Improvements

Separate State from Interaction: Ensure that toggling a feature only changes its on/off state, not other tool interactions. Configurable Defaults: Add startup settings so users can preset which toggles should open in an "on" state by default.

Nested Menu Logic

Action vs. Tool Prioritization: Nested actions should not override active tools unless clearly intended by the user. Clicking a tool in a nested menu should both execute it and prioritize it visually.

Visual & Hotkey Alignment

Update Highlighting: When a hotkey changes the active tool, the toolbar visuals must change in tandem.

OS

s

Node version

s

Browser

s

jenny-hm-lee commented 8 months ago

@sedghi , I don't follow above exactly. It seems like this work is trying to address this bug (#3911 ) also. I have assigned someone to look at #3911 internally. Please let me know if there is overlap.

sedghi commented 8 months ago

I don't see an overlap

sedghi commented 8 months ago

@wayfarer3130 Do you have anything in progress to solve some of these by any chance that Ibrahim can take over?

sedghi commented 5 months ago

We just release the OHIF 3.8, you can find more details here https://ohif.org/release-notes/3p8/ If you still encounter this issue in 3.8, please re-open this.

OHIF / Viewers