Skip to content
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

Remember the forcefully disabled tool and reenable it if possible #6362

Merged
merged 6 commits into from Aug 3, 2022

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 1, 2022

This PR tackles the problem described in #6293. When shortly disabling the segmentation layer via shortcut 3 to only see the color data the volume tool was deselected. The user then had to manually reselect the previous volume tool. To speed up annotating, the forcefully disabled tool is now remembered and reselected again if it is no longer disabled and no other tool has been selected after this tool was disabled.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open an annotation with a segmentation layer.
  • Select the brush tool and hit 3 on the keyboard to disable the segmentation layer.
  • The move tool should now be selected as the brush is disabled.
  • Hit 3 again and the brush tool should now be automatically selected.
  • Do the same for the other volume tools. This should also work.
  • Now select the brush tool, hit 3, and then select the skeleton tool. Hitting 3 again should not select the brush tool.
  • At last, select the brush tool, hit 3, select the skeleton tool, and then select the move tool again. Hitting 3 again should not select the brush tool as the user intentionally changed the tools in between disabling and enabling the segmentation layer.

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Aug 1, 2022

(Should probably be implemented as a saga)

Originally posted by @philippotto in #6293 (comment)

Philipp suggested implementing this in a saga. But upon reading the code it was far simpler to implement the reenabling behaviour directly next to the code that disables the active tool. Thus this solution is likely to have less code and also be more understandable as the disabling and reenabling code is at one location.
That's why this feature is not implemented as a saga. ^^

The downside I noticed is that testing this feature is hard as it is implemented in the toolbar. But in my opinion automated tests are not needed here.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this solution is quite elegant :)

There is one use case it doesn't solve which is if tools are disabled because the user zoomed out too far. However, since this is not mentioned in the original issue, I would say it's fine to merge this as-is 👍

CHANGELOG.unreleased.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remember active tool during toggle segmentation
2 participants