search

mlexchange / mlex_highres_segmentation

A Dash interface for ML-based segmentation of user-annotated large multi-dimensional image data
Other
5 stars 4 forks source link

✨ select classes using keybinds #131

Closed danton267 closed 1 year ago

danton267 commented 1 year ago

when a class is selected it will create a notification keybinds 1-9 select classes if keybind is pressed and there are not that many classes, nothing happens if the keybind for an already selected class is pressed, nothing happens when a class is created and automatically selected - it creates a notification when a class is deleted and a different class is selected - it creates notification

github-actions[bot] commented 1 year ago

Staging application has been deployed and is available at: https://dash5-services.plotly.host/ml-exchange-staging Production app name: ml-exchange Current branch name: class-selection-keybind Commit: 67b5e34090d1b141f0e137d4d3fc00b79ea7f56b

cleaaum commented 1 year ago

Looks good to me, works well! ✨ However I had purposely removed the notification for when a class was selected for two reasons: 1) I felt it was redundant since it is already obvious in the UI which class is selected 2) The notification gets triggered when you hide/edit/delete a class. It doesn't seem to make sense in this case and causes too many notifications imo.

I think the solution would be to look at the ctx and only trigger the notification if a user actually selects a class, or go back to removing the notification.

cleaaum commented 1 year ago

Another small note: I will most likely be merging this PR today, and I've create a function that generates the notification since they are used quite a bit.

See here: https://github.com/mlexchange/mlex_highres_segmentation/pull/119/files#diff-a1f6dd6d8853fe1cf1c95a28cc569e6e5eeeb07b8948c2ecd64364ebcb60e3cbR174

Could be nice to use the function before merging this in!

danton267 commented 1 year ago

Yeah, the "too many notifications" was why I created #132.

I can make it so that notifications only trigger when a keybind is used. We do make a notification whenever the use manually selects an annotation tool too, so if we do make the class notification work with only keybinds, it might be a good idea to make the remainder of the notifications pop up only when keybinds are used, to keep it consistent.

cleaaum commented 1 year ago

Yeah I actually really like that! because sometimes with keybinds you accidentally press them, so it's really nice to get the notification feedback. And we could probably close 132 in this case .

cleaaum commented 1 year ago

New updates look good to me! Just using the notification-generating function is missing. That code is merged now so its accessible:)