For mozilla-mobile/fenix#11739: Fix tabcounter flicker #7446

hkaancaliskan · 2020-06-19T18:17:21Z

Warning: I'm amateur. Double check everything.

I've fixed tab counter flicker issue in fenix and @gabrielluong wanted to upstream that changes to ac.
I'm not good with components. Hope it's good to go :)

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 · 2020-06-19T18:31:22Z

Codecov Report

Merging #7446 into master will increase coverage by 0.03%.
The diff coverage is 93.50%.

@@             Coverage Diff              @@
##             master    #7446      +/-   ##
============================================
+ Coverage     77.62%   77.66%   +0.03%     
+ Complexity     5138     5134       -4     
============================================
  Files           690      689       -1     
  Lines         24985    24949      -36     
  Branches       3690     3675      -15     
============================================
- Hits          19394    19376      -18     
+ Misses         4072     4062      -10     
+ Partials       1519     1511       -8

Impacted Files	Coverage Δ	Complexity Δ
...ava/mozilla/components/ui/tabcounter/TabCounter.kt	`88.81% <93.50%> (+8.97%)`	`13.00 <6.00> (ø)`
...ozilla/components/lib/fetch/okhttp/OkHttpClient.kt

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 6958939...f2a2eb2. Read the comment docs.

gabrielluong · 2020-06-19T18:39:50Z

Thanks for doing this. On a first glance, I am guessing we probably copied and paste the files directly. We have some significant formatting changes probably as a result of downstream changes that we might want to fix here (4 spaces vs 8 spaces for tabbing).

I think we should only focus on your changes of fixing the count display logic in this PR instead of uplifting everything from Fenix in one go and make those changes either in a separate commit or PR for #2743.

One of the benefits of upstreaming these changes is that we do have test coverage for this component which didn't make it into Fenix. You will also need to update TabCounterTest.kt

hkaancaliskan · 2020-06-19T18:51:19Z

Thanks for doing this. On a first glance, I am guessing we probably copied and paste the files directly. We have some significant formatting changes probably as a result of downstream changes that we might want to fix here (4 spaces vs 8 spaces for tabbing).

I think we should only focus on your changes of fixing the count display logic in this PR instead of uplifting everything from Fenix in one go and make those changes either in a separate commit or PR for #2743.

One of the benefits of upstreaming these changes is that we do have test coverage for this component which didn't make it into Fenix. You will also need to update TabCounterTest.kt

Should i roll back changes other than count display logic?

hkaancaliskan · 2020-06-19T18:55:37Z

Thanks for doing this. On a first glance, I am guessing we probably copied and paste the files directly. We have some significant formatting changes probably as a result of downstream changes that we might want to fix here (4 spaces vs 8 spaces for tabbing).

I think we should only focus on your changes of fixing the count display logic in this PR instead of uplifting everything from Fenix in one go and make those changes either in a separate commit or PR for #2743.

One of the benefits of upstreaming these changes is that we do have test coverage for this component which didn't make it into Fenix. You will also need to update TabCounterTest.kt

Only lint problem i saw from firefoxCI rtlsupport=true missing in manifest. Fixed. On my build environment ktlint and detekt didn't show that. Idk why.

gabrielluong · 2020-06-19T19:10:24Z

Thanks for doing this. On a first glance, I am guessing we probably copied and paste the files directly. We have some significant formatting changes probably as a result of downstream changes that we might want to fix here (4 spaces vs 8 spaces for tabbing).
I think we should only focus on your changes of fixing the count display logic in this PR instead of uplifting everything from Fenix in one go and make those changes either in a separate commit or PR for #2743.
One of the benefits of upstreaming these changes is that we do have test coverage for this component which didn't make it into Fenix. You will also need to update TabCounterTest.kt

Should i roll back changes other than count display logic?

I imagine changes like removing the line under the box to be its own commit/pr to keep things simple and make it easy to tell what each commit is doing. So, keep that in mind when you are making your commits and PRs and that should make things go really smooth from a review point of view.

components/ui/tabcounter/src/main/java/mozilla/components/ui/tabcounter/TabCounter.kt

hkaancaliskan · 2020-06-19T20:24:02Z

I imagine changes like removing the line under the box to be its own commit/pr to keep things simple and make it easy to tell what each commit is doing. So, keep that in mind when you are making your commits and PRs and that should make things go really smooth from a review point of view.

@gabrielluong I'll close this and open another pr with proper commiting. Don't review.
Also unit test failing repeatedly. I can't find why. Could you please tell me why is this failing?
Edit: I've tried to add text.text = INTERNAL_COUNT.toString() to init. Let's see if it fixes.

Also local unit test doesn't work for me as i wrote on riot ac. Thus I'm trying to fix unit test via FirefoxCI.

hkaancaliskan · 2020-06-19T21:21:39Z

Found out why unit test fail.
There is some kinda order problem. 99 -> 1 -> 100 -> 0
Thus it prints 100 instead of 0 because TabCounter run before and left as 100.

hkaancaliskan · 2020-06-19T21:56:32Z

@gabrielluong unit tests fixed.
Should I make proper commiting on new pr or is this okey?

gabrielluong · 2020-06-19T22:10:44Z

@gabrielluong unit tests fixed.
Should I make proper commiting on new pr or is this okey?

One protip you could do is to squash your commits and force push to your branch which will update the PR.

gabrielluong · 2020-06-19T22:12:11Z

components/ui/tabcounter/src/main/java/mozilla/components/ui/tabcounter/TabCounter.kt

    context: Context,
    attrs: AttributeSet? = null,
    defStyle: Int = 0
 ) : RelativeLayout(context, attrs, defStyle) {

    private val box: ImageView
-    private val bar: ImageView


We don't want the PR to be about removing the bar necessarily since it should be about fixing the tab count. So, I would suggest undo-ing some of these changes and just make sure it is refined to the problem we are trying to solve. We could introduce changes to remove the bar in a different commit/PR.

Just the bar for now need undo-ing or is there another?

I would probably undo most changes that does not touch the count. The PR should look similar to your limited changes on resolving the count like your Fenix PR.

I'm going to close this pr and open a new pr with just the logic? Is this okey?

Yep, organize it however you want to.

hkaancaliskan mentioned this pull request Jun 19, 2020

[Bug] Tab counter flickers mozilla-mobile/fenix#11739

Closed

hkaancaliskan marked this pull request as draft June 19, 2020 18:53

gabrielluong reviewed Jun 19, 2020

View reviewed changes

components/ui/tabcounter/src/main/java/mozilla/components/ui/tabcounter/TabCounter.kt Outdated Show resolved Hide resolved

components/ui/tabcounter/src/main/java/mozilla/components/ui/tabcounter/TabCounter.kt Show resolved Hide resolved

gabrielluong reviewed Jun 19, 2020

View reviewed changes

Hakkı Kaan Çalışkan added 2 commits June 20, 2020 01:51

For mozilla-mobile/fenix#11739: Fix tabcounter flicker

c1e2256

undo removing of tabcounter_bar

f2a2eb2

hkaancaliskan closed this Jun 19, 2020

hkaancaliskan deleted the tabcounter branch June 19, 2020 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For mozilla-mobile/fenix#11739: Fix tabcounter flicker #7446

For mozilla-mobile/fenix#11739: Fix tabcounter flicker #7446

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

codecov bot commented Jun 19, 2020 •

edited

Loading

gabrielluong commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020

gabrielluong commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

hkaancaliskan commented Jun 19, 2020

gabrielluong commented Jun 19, 2020

gabrielluong Jun 19, 2020

hkaancaliskan Jun 19, 2020

gabrielluong Jun 19, 2020

hkaancaliskan Jun 19, 2020 •

edited

Loading

gabrielluong Jun 19, 2020

For mozilla-mobile/fenix#11739: Fix tabcounter flicker #7446

For mozilla-mobile/fenix#11739: Fix tabcounter flicker #7446

Conversation

hkaancaliskan commented Jun 19, 2020 • edited Loading

Pull Request checklist

After merge

codecov bot commented Jun 19, 2020 • edited Loading

Codecov Report

gabrielluong commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020

gabrielluong commented Jun 19, 2020

hkaancaliskan commented Jun 19, 2020 • edited Loading

hkaancaliskan commented Jun 19, 2020 • edited Loading

hkaancaliskan commented Jun 19, 2020

gabrielluong commented Jun 19, 2020

gabrielluong Jun 19, 2020

Choose a reason for hiding this comment

hkaancaliskan Jun 19, 2020

Choose a reason for hiding this comment

gabrielluong Jun 19, 2020

Choose a reason for hiding this comment

hkaancaliskan Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

gabrielluong Jun 19, 2020

Choose a reason for hiding this comment

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

codecov bot commented Jun 19, 2020 •

edited

Loading

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

hkaancaliskan commented Jun 19, 2020 •

edited

Loading

hkaancaliskan Jun 19, 2020 •

edited

Loading