Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #7313: Use ThumbnailLoader for the TabViewHolder #7356

Merged
merged 4 commits into from
Jun 12, 2020

Conversation

jonalmeida
Copy link
Contributor

@jonalmeida jonalmeida commented Jun 12, 2020

Instead of trying to inline the thumbnail images from disk into the TabsTrayPresenter, we can load them from the ThumbnailStorage via the ThumbnailLoader and rely on the TabsTrayPresenter to consume new thumbnail updates only from the store.

This fixes some perf issues, inconsistencies, and duplicate updates to the tabs tray.


Corresponding Fenix PR: mozilla-mobile/fenix#11510

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #7356 into master will increase coverage by 0.09%.
The diff coverage is 73.68%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7356      +/-   ##
============================================
+ Coverage     77.41%   77.51%   +0.09%     
+ Complexity     5072     5051      -21     
============================================
  Files           675      670       -5     
  Lines         24787    24727      -60     
  Branches       3663     3659       -4     
============================================
- Hits          19190    19168      -22     
+ Misses         4090     4051      -39     
- Partials       1507     1508       +1     
Impacted Files Coverage Δ Complexity Δ
...a/mozilla/components/browser/icons/BrowserIcons.kt 66.33% <ø> (ø) 10.00 <0.00> (ø)
...ponents/feature/tabs/tabstray/TabsTrayPresenter.kt 94.11% <ø> (-0.17%) 10.00 <0.00> (ø)
...la/components/support/images/loader/ImageLoader.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...nents/browser/thumbnails/loader/ThumbnailLoader.kt 55.55% <55.55%> (ø) 5.00 <5.00> (?)
...zilla/components/browser/tabstray/TabViewHolder.kt 93.75% <100.00%> (+0.89%) 1.00 <0.00> (ø)
...mozilla/components/browser/tabstray/TabsAdapter.kt 87.50% <100.00%> (+1.13%) 11.00 <1.00> (ø)
...nts/browser/tabstray/thumbnail/TabThumbnailView.kt 100.00% <100.00%> (ø) 3.00 <2.00> (ø)
...ozilla/components/support/images/CancelOnDetach.kt 100.00% <100.00%> (ø) 3.00 <1.00> (?)
...port/android/test/espresso/matcher/ViewMatchers.kt
...ts/feature/intent/processing/TabIntentProcessor.kt
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e12caf...1f143e1. Read the comment docs.

@jonalmeida jonalmeida force-pushed the issue-7313 branch 2 times, most recently from 153bbce to dc99982 Compare June 12, 2020 16:29
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
)
}
},
delegate: Observable<TabsTray.Observer> = ObserverRegistry()
Copy link
Contributor

Choose a reason for hiding this comment

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

would putting this parameter after the closure prevent us from using the trailing closures when initializing the TabsAdapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boek I explicitly wanted to avoid that since we've had issues (mozilla-mobile/fenix#10495) where the scope of the trailing closure did not make it obvious it was being invoked during the onCreateViewHolder call of the adapter and that could cause crashes depending on what work was done in that closure.

I thought it was better to have an explicit named param in that case to avoid regressions.

@jonalmeida
Copy link
Contributor Author

bors r=gabrielluong,boek

@bors
Copy link

bors bot commented Jun 12, 2020

Build succeeded:

@bors bors bot merged commit ee34a97 into mozilla-mobile:master Jun 12, 2020
@jonalmeida jonalmeida deleted the issue-7313 branch June 12, 2020 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🕵️‍♀️ needs review PRs that need to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TabThumbnailView should always scale to the entire width Thumbnail changes update the tabs tray too often
3 participants