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 using CoilImage with an implicit size #61

Merged
merged 3 commits into from Aug 17, 2020
Merged

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Aug 14, 2020

Currently CoilImage only works when you specific a size Modifier, since it waits for onPositioned to be called, and only starts a Coil request when it has a non-zero size. If no Modifier is provided, the CoilImage will have a zero size and we never execute a Coil request.

Fixed by moving back to WithConstraints to move the request size logic to be based on the incoming constraints, rather than the final size, which is probably more correct anyway.

Currently CoilImage only works when you specific a size
Modifier, since it waits for onPositioned to be called,
and only starts a Coil request when it has a non-zero
size. If no Modifier is provided, the CoilImage will
have a zero size and we never execute a Coil request.

Fixed by moving back to WithConstraints to move the request
size logic to be based on the incming constraints, rather
than the final size, which is probably more correct anyway.
Copy link
Contributor

@handstandsam handstandsam left a comment

Choose a reason for hiding this comment

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

Nice, thanks for addressing this!

Can you create a new constant called UNSPECIFIED which can be Int.MAX_VALUE behind the scenes? It makes for more readable code IMO. I would suggest null, but I see that all these things are not nullable so an UNSPECIFIED reads fine in this case.

@chrisbanes
Copy link
Contributor Author

chrisbanes commented Aug 14, 2020

Can you create a new constant called UNSPECIFIED which can be Int.MAX_VALUE behind the scenes?

Sounds good, done.

Copy link
Contributor

@handstandsam handstandsam left a comment

Choose a reason for hiding this comment

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

Update looks good, just saw one typo in the Javadoc. Up to you if you want to address. Minor.

coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt Outdated Show resolved Hide resolved
@probot-auto-merge probot-auto-merge bot merged commit d514ec0 into main Aug 17, 2020
@probot-auto-merge probot-auto-merge bot deleted the cb/coil-implicit branch August 17, 2020 08:00
chrisbanes added a commit that referenced this pull request Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants