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

fix(discoverability): set mode to REGION instead of NONE #1260

Merged
merged 6 commits into from
Sep 22, 2020

Conversation

ChenCodes
Copy link
Contributor

@ChenCodes ChenCodes commented Sep 18, 2020

  • update toggleAnnotationMode to be called with REGION instead of NONE if FF is on
  • update tests

Demo:
ezgif com-video-to-gif (5)

@ChenCodes ChenCodes requested a review from a team as a code owner September 18, 2020 21:40
@@ -915,7 +917,9 @@ class BaseViewer extends EventEmitter {

disableAnnotationControls() {
if (this.annotator && this.annotationControls && this.areNewAnnotationsEnabled()) {
this.annotator.toggleAnnotationMode(AnnotationMode.NONE);
this.annotator.toggleAnnotationMode(
Copy link
Contributor Author

@ChenCodes ChenCodes Sep 18, 2020

Choose a reason for hiding this comment

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

Something that I noticed is that we have two toggleAnnotationMode method calls being made in the
handleFullscreenEnter -> disableAnnotationControls flow. My thinking is that either the first or second is redundant?

Copy link
Contributor

@mxiao6 mxiao6 Sep 18, 2020

Choose a reason for hiding this comment

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

Good point. I think the first is redundant. @jstoffan can you confirm?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure. It seems like these changes somewhat defeat the purpose of the original code, which was to forcibly disallow annotation creation. What effect does this have on creating annotations on rotated images, for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My change was not in the right location, so I have moved it over to handleFullScreenExit instead.

@mxiao6
Copy link
Contributor

mxiao6 commented Sep 21, 2020

I notice there is another this.annotator.toggleAnnotationMode call in handleAnnotationControlsEscape() and I believe it is also causing bug. Basically, I think we should wrap this.annotator.toggleAnnotationMode into another function with discovery FF logic and change all of the current this.annotator.toggleAnnotationMode calls to the new function. Does that sound reasonable?

@ChenCodes
Copy link
Contributor Author

I notice there is another this.annotator.toggleAnnotationMode call in handleAnnotationControlsEscape() and I believe it is also causing bug. Basically, I think we should wrap this.annotator.toggleAnnotationMode into another function with discovery FF logic and change all of the current this.annotator.toggleAnnotationMode calls to the new function. Does that sound reasonable?

I'm not sure if we want to move all of the this.annotator.toggleAnnotation to the new function. The reason is that in some locations, we always want this.annotation.toggleAnnotationMode to be called with NONE because we are explicitly defining that the user should not be able to create regions. In other locations, we do want to set the default to be REGION (for example, in handleFullScreenExit, we do want to call it this.annotation.toggleAnnotationMode with REGION here if FF is on because we want the default mode to be REGION.

@mergify mergify bot merged commit 6ccbb55 into box:master Sep 22, 2020
@ChenCodes ChenCodes deleted the exiting-fullscreen-changes-mode-to-none branch September 22, 2020 17:31
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.

4 participants