Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Prevent zoom percentage lower than 1% - #60

Merged
thomasjo merged 1 commit into
masterfrom
tj-enforce-positive-zoom
Jul 4, 2016
Merged

Prevent zoom percentage lower than 1%#60
thomasjo merged 1 commit into
masterfrom
tj-enforce-positive-zoom

Conversation

@thomasjo

@thomasjo thomasjo commented Jul 3, 2016

Copy link
Copy Markdown
Contributor

Fixes #59

/cc @atom/feedback

@mnquintana

Copy link
Copy Markdown
Contributor

🚢

@thomasjo
thomasjo merged commit f351d58 into master Jul 4, 2016
@thomasjo
thomasjo deleted the tj-enforce-positive-zoom branch July 4, 2016 04:32
@Ben3eeE

Ben3eeE commented Jul 4, 2016

Copy link
Copy Markdown
Contributor

I don't think this fixes #59, if you look at my gif of repro the zoom percentage is 23/38 when it gets stuck not very small.

I'm just guessing here but does the zoom factor multiply with the displayed size of the image and chrome doesn't display the image as 3.75x3.75 but rather as 3x3 and the same goes for 2.25x2.25 being displayed as 3x3 until you resize the window then it is 3x2, 2x3 or 2x2 depending on how you resized?

Some images:
2 25 size says 3x3
3 75 size says 3x3
2 25 after resize 3x2

@thomasjo

thomasjo commented Jul 4, 2016

Copy link
Copy Markdown
Contributor Author

I'm pretty sure this fixes the problem. At least when I tried to reproduce the problem, what I experienced was a rounding off "error" which occured when continuously zooming out. The result was the zoom percentage being rounded off to 0%, after which there was no way to recover since we would always multiply by something that was numerically equivalent to zero.

Please test if this fixes the problem you experienced. At the very least this fixes a tangential issue.
If your problem persists after this fix, please let me know.

@Ben3eeE

Ben3eeE commented Jul 4, 2016

Copy link
Copy Markdown
Contributor

Sorry, I should have mentioned that I did test and my problem does persist. Can you try to zoom out with the spec image in the image-view repository and see what results you get? https://github.com/atom/image-view/blob/master/spec/fixtures/binary-file.png

For me it gets stuck at 23% zooming out and I can't zoom back in more than once up to 38% or sometimes it gets stuck at 15% zooming out and I can't zoom back in more than once up to 25%. Depending on the size of the Atom window?

But I am using dev mode out of safe mode with Atom 1.8.0 and a weird computer when testing, so maybe something is wrong on my side. I can test on latest master later to see what results I get there.

d

@thomasjo

thomasjo commented Jul 4, 2016

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up! 🙇
I can indeed reproduce what you're describing — will fix this issue as well.

@simurai

simurai commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

Currently the scaling is based on the "physical" width()/height() of an image: https://github.com/atom/image-view/blob/master/lib/image-editor-view.coffee#L127-L128. But when very small and maybe some kind of rounding to whole pixels, it throws off the original aspect ratio?

Maybe we can remember the original size of an image and then scale based on that?

@thomasjo

thomasjo commented Jul 6, 2016

Copy link
Copy Markdown
Contributor Author

Problem is the current system is a little bit flawed due to rounding-off errors and such. I'm working on a PR that transitions to a system with predefined "zoom levels", and thus zooming in and out simply transitions betweens those percentages.


Side note: this means we always use the original width and height, hence we are not subjected to accumulated rounding-off errors, etc.

@Ben3eeE

Ben3eeE commented Jul 6, 2016

Copy link
Copy Markdown
Contributor

to a system with predefined "zoom levels"

How would this work with the suggested enhancements of manually writing zoom level? Discussed here:
#40 (comment)

@thomasjo

thomasjo commented Jul 6, 2016

Copy link
Copy Markdown
Contributor Author

That will still work, along the same lines as "zoom to fit". Of course we'd need to determine what should happen when the user clicks zoom in or out after having set a custom percentage, but that's trivial.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants