Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/alignment #76
Feature/alignment #76
Changes from 9 commits
8c92979
6b10f4b
0be7374
06e618c
c7a154a
c19fe3d
9af4050
098bd47
7df592b
08cda64
e37522d
ffc6e1d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] so it is only applied when the content is smaller than the container (e.g.
zoom < 1
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will address all these in a while. But wanted to say that
zoom < 1
does not necessarily mean that content is smaller than container. It does only with CENTER_CROP or CENTER_INSIDE transformations and even then, it depends on the axis.Just saying because I have seen this check (zoom < 1F) in the pivot function, and I don't know if it's correct or not. I'm tempted to say that it's not, and I also wonder if that should be changed according to the alignment? Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes that makes sense. I guess I was lucky in my testing.
Yes I think so too. As i mentioned in the comment there I initially wanted to base it on the transformationGravity but as the alignment will be the new thing after this PR it should be based on it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what that function should return? It's not 100% clear to me, I didn't investigate, but I can try to update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the function decides what pivot point should be used for the zoom animation based on what corner of the screen is currently overscrolled (or will be, with the target zoom applied). I did this because applying the pan fix and the zoom fix at the same time in an animation when overscrolled while zoomed in a bit (specifically overpinching and overscrolling at the same time in a single pinch gesture) didn't result in the expected target position and/or a funky animation. Implementing this was a bit of trial and error tbh and I think this method is only applicable when the content is zoomed in meaning that at most two sides of the content are overscrolled/have visible gray areas. Because of that I added the
zoom < 1
condition which just takes the center of the screen as the pivot point when the view has more than two gray areas around it (which doesn't seem to be the correct way of figuring that out). My current guess would be that this should be based on the alignment instead of just using the center when there are more than two gray areas but this whole method (and it's purpose) may require a complete rethinking.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. Looks like it might take some time, I think I will merge this now and we'll fix later.