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

Glide - Use current drawable while loading new static map image (PSF-979) #6103

Merged
merged 2 commits into from May 20, 2022

Conversation

onurays
Copy link
Contributor

@onurays onurays commented May 20, 2022

Interestingly, Glide is clearing current image before the new image is loaded which causes a flicker on timeline item. This PR suggests to use current drawable as placeolder while loading the new static map image.

@onurays onurays requested review from a team, yostyle and Claire1817 and removed request for a team May 20, 2022 09:22
@github-actions
Copy link

Unit Test Results

124 files  ±0  124 suites  ±0   2m 2s ⏱️ -2s
217 tests ±0  217 ✔️ ±0  0 💤 ±0  0 ±0 
714 runs  ±0  714 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 468cc30. ± Comparison against base commit b547a49.

@@ -75,6 +75,7 @@ abstract class AbsMessageLocationItem<H : AbsMessageLocationItem.Holder> : AbsMe
GlideApp.with(holder.staticMapImageView)
.load(location)
.apply(RequestOptions.centerCropTransform())
.placeholder(holder.staticMapImageView.drawable)
Copy link
Member

Choose a reason for hiding this comment

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

This fix seems to come from this suggestion: bumptech/glide#2505 (comment)

It's not recommended to use that in RecyclerView, which is the case here (see the comments below).

I should work, but may lead to some issue.

Not a blocker anyway.

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 fix seems to come from this suggestion: bumptech/glide#2505 (comment)

Exactly, I couldn't find any other solutions. I could understand the below comment if we were setting the actual source but we are just setting the placeholder.

  • We are going to have a test session next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm agree with @bmarty about this fix. Further tests are needed.

@onurays onurays merged commit 109b381 into develop May 20, 2022
@onurays onurays deleted the feature/ons/fix_live_location_flickering branch May 20, 2022 15:36
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=20 failures=0 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=3 failures=0 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=5 failures=1 errors=0 skipped=0
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

@mnaturel mnaturel changed the title Glide - Use current drawable while loading new static map image Glide - Use current drawable while loading new static map image (PSF-979) May 23, 2022
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.

None yet

3 participants