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 #1782 and part of #1781: Fix text view image loading issues #1797

Merged
merged 2 commits into from
Sep 4, 2020

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Sep 4, 2020

Fixes #1782
Partly fixes #1781 (for online cases)

There seems to be a not well-documented or seen bug in AOSP wherein TextViews with HTML that only contains an ImageSpan and no other visible text can result in the TextView seemingly being ignored for layout & measurement. This results in our image computation for ImageSpan failing. The hack involves adding spaces around the image (technically just 1 is needed, but 2 are added to keep the image horizontally centered) which triggers Android to properly lay out the view & allow our image computation to kick-in. I filed #1796 to track finding a better long-term solution.

Note that this also includes another semi-hacky workaround: getting Android to layout the view is only part of the solution. In cases when TextView is using wrap_content, we don't have a true maximum to use when ensuring the image doesn't exceed its container's bounds. For the purpose of keeping things simple for now, we're assuming that in such cases we can limit the TextView's dimensions to that of its parent's minus established right/left margins. This should be correct in all cases (unless a maxWidth is set smaller than the parent--we don't account for this right now).

No new tests were added for the UrlImageParser work. #277 is tracking adding test cases for the parser (there's a bit too much to do there for me to do it quickly, so I deferred to that issue, instead).

Thanks to @rt4914 for the early investigation with the wrap_content part.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @BenHenning
caching is fast, just image loads a bit slow as earlier when we run the app for the first time only
Screenshot_1599212387

Screenshot_1599212395

@BenHenning
Copy link
Sponsor Member Author

BenHenning commented Sep 4, 2020

I noticed that, too. I think it might have to do with extra layout steps needed to do the computation (which means we probably do have a regression which is why we haven't noticed this bug before). #434 should probably include some investigations in this area.

Also, fortunately offline caching makes the loading super fast so it should be a better experience for alpha users.

Thanks @anandwana001!

@anandwana001 anandwana001 removed their assignment Sep 4, 2020
@BenHenning BenHenning merged commit 3c3a010 into develop Sep 4, 2020
@BenHenning BenHenning deleted the fix-text-view-image-loading-issues branch September 4, 2020 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image missing in multiplication lesson Images are failing to load sometimes in the exploration player
2 participants