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

For #6895: Add optional itemDecoration in browser tabs tray. #6896

Merged
merged 2 commits into from
May 7, 2020
Merged

For #6895: Add optional itemDecoration in browser tabs tray. #6896

merged 2 commits into from
May 7, 2020

Conversation

mcarare
Copy link
Contributor

@mcarare mcarare commented May 7, 2020


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.

@mcarare mcarare changed the title Item decoration For #6895: Add optional itemDecoration in browser tabs tray. May 7, 2020
@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #6896 into master will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6896      +/-   ##
============================================
- Coverage     77.33%   77.21%   -0.12%     
+ Complexity     4817     4776      -41     
============================================
  Files           646      641       -5     
  Lines         23570    23402     -168     
  Branches       3469     3434      -35     
============================================
- Hits          18227    18070     -157     
+ Misses         3894     3889       -5     
+ Partials       1449     1443       -6     
Impacted Files Coverage Δ Complexity Δ
...lla/components/browser/tabstray/BrowserTabsTray.kt 73.52% <100.00%> (+1.65%) 3.00 <0.00> (+1.00)
...nts/lib/publicsuffixlist/PublicSuffixListLoader.kt
...nents/lib/publicsuffixlist/PublicSuffixListData.kt
...a/components/lib/publicsuffixlist/ext/ByteArray.kt
...illa/components/lib/publicsuffixlist/ext/String.kt
...omponents/lib/publicsuffixlist/PublicSuffixList.kt
...components/support/sync/telemetry/SyncTelemetry.kt 87.09% <0.00%> (+1.38%) 41.00% <0.00%> (ø%)

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 d153d0d...63b71a0. Read the comment docs.

Copy link
Contributor

@psymoon psymoon left a comment

Choose a reason for hiding this comment

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

Looks good, just a small nit.

docs/changelog.md Outdated Show resolved Hide resolved
@psymoon psymoon self-assigned this May 7, 2020
@jonalmeida
Copy link
Contributor

@mcarare The confusion seems to have come from the lambda in the TabsAdapter that had a tabsTray field. BrowserTabsTray extends from a RecyclerView so you can still make similar errors with this fix.

I agree we need a better API though. 🙂

@mcarare
Copy link
Contributor Author

mcarare commented May 7, 2020

@jonalmeida Yes, I think we should take into considerations more use cases and possible pitfalls of using this API. The change in this PR came as a fix to the crash that happens in fenix nightly, caused by the fact that tabsTray.addItemDecoration(decoration) was called before calling the BrowserTabsTray constructor.

@jonalmeida
Copy link
Contributor

Adding the item decoration was not the cause for that bug, but that it was in the scope of the TabsAdapter.viewHolderProvider which is invoked when a new view holder is created.

Maybe we should look into making a better TabsAdapter too?

@mcarare
Copy link
Contributor Author

mcarare commented May 7, 2020

@jonalmeida Im not sure we are talking about the same crash. I was referring to this one: mozilla-mobile/fenix#10453

@jonalmeida
Copy link
Contributor

@jonalmeida Im not sure we are talking about the same crash. I was referring to this one: mozilla-mobile/fenix#10453

Yes, this was the fix for it. :)

mozilla-mobile/fenix#10495

@mcarare
Copy link
Contributor Author

mcarare commented May 7, 2020

@jonalmeida Im not sure we are talking about the same crash. I was referring to this one: mozilla-mobile/fenix#10453

Yes, this was the fix for it. :)

mozilla-mobile/fenix#10495

Yeah, I already had a PR for that: mozilla-mobile/fenix#10476...

@jonalmeida
Copy link
Contributor

@jonalmeida Im not sure we are talking about the same crash. I was referring to this one: mozilla-mobile/fenix#10453

Yes, this was the fix for it. :)
mozilla-mobile/fenix#10495

Yeah, I already had a PR for that: mozilla-mobile/fenix#10476...

Oh no, I'm sorry! I didn't see any discussion happening on Riot or Slack, so I put up a PR. Apologise for that!

I'm still unsure this change will avoid that bug, but let's try.

@mcarare
Copy link
Contributor Author

mcarare commented May 7, 2020

@jonalmeida Np. I have already tried it with local AC build, it works :)

@jonalmeida
Copy link
Contributor

bors r=psymoon

bors bot pushed a commit that referenced this pull request May 7, 2020
6896: For #6895: Add optional itemDecoration in browser tabs tray. r=psymoon a=mcarare



Co-authored-by: mcarare <[email protected]>
@bors
Copy link

bors bot commented May 7, 2020

Build failed:

@psymoon
Copy link
Contributor

psymoon commented May 7, 2020

bors retry

@bors
Copy link

bors bot commented May 7, 2020

Build succeeded:

  • complete-push

@bors bors bot merged commit 9adccaa into mozilla-mobile:master May 7, 2020
@gabrielluong gabrielluong linked an issue May 8, 2020 that may be closed by this pull request
@mcarare mcarare deleted the item-decoration branch June 3, 2020 10:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional parameter Item Decoration in BrowserTabsTray
3 participants