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

Disable brush in mags higher than mag8 #5017

Merged
merged 13 commits into from
Jan 12, 2021
Merged

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Jan 5, 2021

URL of deployed dev instance (used for testing):

Steps to test:

  • open a tracing, switch to brush tool
  • ensure that brush is usable in mags 1 to 8
  • the brush tool should get disabled (and another tool should be activated) automatically as soon as one zooms to an unusable mag

Issues:


@philippotto philippotto self-assigned this Jan 5, 2021
@philippotto philippotto requested a review from daniel-wer January 5, 2021 12:31
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.

Works like a charm, only two minor non-blocking suggestions :)

@@ -180,7 +183,7 @@ const getExplanationForDisabledVolume = (
return "Volume annotation is disabled while the merger mode is active.";
}

if (!isLabelingPossible) {
if (!isSegmentationVisibleForMag) {
Copy link
Member

Choose a reason for hiding this comment

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

One thing I noticed during testing is that this boolean is also false if the segmentation layer is simply disabled (for example, using 3). Could you adapt the message accordingly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Since the existing logic around the disabled tool tooltip (i.e., which text to show) was a bit convoluted, I refactored the corresponding code which made it easier to integrate the case you mentioned. Also, the automatic switching of the tool (if the current one gets disabled), got a bit DRYer due to the refactoring.
Feel free to have another look!

@bulldozer-boy bulldozer-boy bot merged commit eec0972 into master Jan 12, 2021
@bulldozer-boy bulldozer-boy bot deleted the disable-brush-in-high-mags branch January 12, 2021 10:38
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.

Brushing in high mags (e.g., > mag 16) is slow
2 participants