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.

Sign up for GitHub

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: Ensure that annotation scale is never being set to auto #208

Merged
merged 2 commits into from
Jul 11, 2017
Merged

Fix: Ensure that annotation scale is never being set to auto #208

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6697f68
Fix: Ensure that annotation scale is never being set to auto
Jul 11, 2017
0d98c4a
Merge branch 'master' into master
pramodsum Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/viewers/doc/DocBaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,7 +1030,7 @@ const MOBILE_MAX_CANVAS_SIZE = 2949120; // ~3MP 1920x1536

// Set scale to current numerical scale & rendered page number
this.emit('scale', {
scale: this.pdfViewer.currentScaleValue,
scale: this.pdfViewer.currentScale,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would cause the annotation scale to be set as "auto" until the document is zoomed in/out for the first time. this.pdfViewer.currentScale is a usable value for annotator scale

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually what was being set before. It got swapped in a previous change

Copy link

Choose a reason for hiding this comment

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

Is the difference that currentScaleValue is always a number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. currentScaleValue is usually just "auto" until the file is zoomed in/out at least once

Copy link
Contributor

Choose a reason for hiding this comment

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

In BaseViewer.scaleAnnotations, the line
this.annotator.setScale(data.scale);
should be above
if (Object.keys(this.annotator.threads).length === 0) { return; }
so that the data-scale attribute is always set, regardless of whether or not annotations have been fetched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting the data-scale automatically triggers a bunch of annotations methods to get called. I'm not sure it makes sense to trigger those (even if there's not much it ends up doing when there are 0 annotations).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Minh, but I think the change will have to happen when we have some refactor time. My assumption is that a bunch of events are fired off inside of setScale() that might cause some funkiness if removed/blocked from happening :S

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It ends up triggering a few other methods which don't quite make sense to trigger unless the annotations have already been fetched. It might end up becoming a part of our larger refactor :-/

Copy link
Contributor

@Minh-Ng Minh-Ng Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What should happen when you open a preview when your browser size is half the screen? The scaling will not be auto (the actual value will be something fractional) but the data-scale field will still be auto.

Shouldn't methods listening to scale be dependent on scale? Then the annotation methods that get triggered should do nothing when it is determined that there is nothing relevant for the method to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this to make sure just now and it seems like it's being set as 1 at the start and when you resize it from there, this change forces it to a numerical value so it should never be set as "auto"

pageNum: pageNumber
});

Expand Down