For #15402: Hide ETP pop-up if the toolbar is not visible #15667

sijanr · 2020-10-04T03:02:20Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

firefoxci-taskcluster · 2020-10-04T03:02:37Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt

Mugurell

Thank you for looking into this!

For a complete fix we need the same issue also resolved for when the toolbar is set at top (to know about this I think you can use a AppBarLayout.OnOffsetChangedListener)

Changes after which TrackingProtectionOverlayTest will probably need to be updated.

…is not visible

sijanr

The issue with adding the listener would be there's already one added for the app bar in BrowserToolbarView.kt.

fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarView.kt

Line 107 in ad2b99f

app_bar.addOnOffsetChangedListener(offsetChangedListener)

So, I checked the parent of the toolbar instead.

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt

Mugurell · 2020-10-06T07:07:20Z

Thank you!
It looks and functions nicely!
Just one improvement to be made regarding knowing if we have a top or bottom toolbar.
Some small tests just for the return would be nice but I see the showTrackingProtectionOnboarding does quite a lot so fully testing this could be done in a later ticket.

sijanr

Yeah I agree with you that the code couldn't be easily read. I found a property to check the toolbar's position in Settings class and included it in the check because it appeared more readable. Thanks for your help.

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt

Mugurell · 2020-10-07T07:39:17Z

Yeah I agree with you that the code couldn't be easily read. I found a property to check the toolbar's position in Settings class and included it in the check because it appeared more readable. Thanks for your help.

Thank you!
It looks indeed better!

One small thing related to a safe cast to be resolved and then I'll merge this.

Mugurell · 2020-10-08T14:12:39Z

bors try

bors · 2020-10-08T14:14:49Z

try

Build succeeded:

Mugurell · 2020-10-08T15:22:13Z

@sijanr
Looks like ktlint fails at

TrackingProtectionOverlay.kt:16:1: Unused import
TrackingProtectionOverlay.kt:61:19: Missing spacing after "if"
TrackingProtectionOverlay.kt:68:23: Missing spacing after "if"

Can you please check and update the patch?

sijanr

I think this should fix the ktlint errors

Mugurell · 2020-10-09T11:03:08Z

bors try

bors · 2020-10-09T11:05:14Z

try

Build succeeded:

Mugurell

LGTM
Thanks for pushing through with this!

Mugurell · 2020-10-09T11:41:31Z

bors r=Mugurell

bors · 2020-10-09T11:41:33Z

👎 Rejected by label

Mugurell · 2020-10-09T11:42:01Z

bors r=Mugurell

15667: For #15402: Hide ETP pop-up if the toolbar is not visible r=Mugurell a=sijanr ![ETP](https://user-images.githubusercontent.com/47872289/95005944-269c6280-05b3-11eb-870e-4bd2ef90bf79.gif) ### Pull Request checklist  - [ ] **Tests**: This PR includes thorough tests or an explanation of why it does not - [x] **Screenshots**: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not - [ ] **Accessibility**: The code in this PR follows [accessibility best practices](https://github.com/mozilla-mobile/shared-docs/blob/master/android/accessibility_guide.md) or does not include any user facing features. In addition, it includes a screenshot of a successful [accessibility scan](https://play.google.com/store/apps/details?id=com.google.android.apps.accessibility.auditor&hl=en_US) to ensure no new defects are added to the product. ### To download an APK when reviewing a PR: 1. click on Show All Checks, 2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark, 3. click on the "Fenix - assemble" task, then click "Run Artifacts". 4. the APK links should be on the left side of the screen, named for each CPU architecture Co-authored-by: sijanr <sijan1.rijal@gmail.com>

codecov-io · 2020-10-09T13:02:27Z

Codecov Report

Merging #15667 into master will increase coverage by 0.01%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##             master   #15667      +/-   ##
============================================
+ Coverage     29.79%   29.80%   +0.01%     
- Complexity     1180     1182       +2     
============================================
  Files           452      452              
  Lines         18394    18401       +7     
  Branches       2376     2381       +5     
============================================
+ Hits           5481     5485       +4     
- Misses        12525    12528       +3     
  Partials        388      388

Impacted Files	Coverage Δ	Complexity Δ
...ix/trackingprotection/TrackingProtectionOverlay.kt	`67.79% <20.00%> (-9.13%)`	`9.00 <4.00> (ø)`
app/src/main/java/org/mozilla/fenix/Config.kt	`50.00% <0.00%> (-4.55%)`	`2.00% <0.00%> (ø%)`
...mponents/searchengine/FenixSearchEngineProvider.kt	`60.71% <0.00%> (+2.67%)`	`14.00% <0.00%> (ø%)`
...in/java/org/mozilla/fenix/settings/SupportUtils.kt	`74.50% <0.00%> (+3.92%)`	`13.00% <0.00%> (+2.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 ec3595d...c6d5108. Read the comment docs.

For mozilla-mobile#15402: Hide ETP pop-up if the toolbar is not visible

5b46267

sijanr requested review from a team as code owners October 4, 2020 03:02

Mugurell reviewed Oct 5, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt Outdated Show resolved Hide resolved

Mugurell suggested changes Oct 5, 2020

View reviewed changes

Mugurell self-assigned this Oct 5, 2020

Mugurell added the pr:needs-changes PRs that need some changes/fixes before they can land label Oct 5, 2020

For mozilla-mobile#15402: Hide ETP pop-up when top or bottom toolbar …

19373c8

…is not visible

sijanr commented Oct 5, 2020

View reviewed changes

Mugurell reviewed Oct 6, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt Outdated Show resolved Hide resolved

For mozilla-mobile#15402: Improve toolbar's position check

6146be4

sijanr commented Oct 6, 2020

View reviewed changes

Mugurell reviewed Oct 7, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/trackingprotection/TrackingProtectionOverlay.kt Outdated Show resolved Hide resolved

For mozilla-mobile#15402: Use safe cast

b4d5975

bors bot pushed a commit that referenced this pull request Oct 8, 2020

Try #15667:

8f4c528

For mozilla-mobile#15402: Fix ktlint fails

c6d5108

sijanr commented Oct 9, 2020

View reviewed changes

bors bot pushed a commit that referenced this pull request Oct 9, 2020

Try #15667:

204190b

Mugurell approved these changes Oct 9, 2020

View reviewed changes

Mugurell removed the pr:needs-changes PRs that need some changes/fixes before they can land label Oct 9, 2020

Mugurell mentioned this pull request Oct 9, 2020

For #15402: Hide ETP pop-up if the toolbar is not visible #15807

Closed

3 tasks

Mugurell merged commit 127d006 into mozilla-mobile:master Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #15402: Hide ETP pop-up if the toolbar is not visible #15667

For #15402: Hide ETP pop-up if the toolbar is not visible #15667

sijanr commented Oct 4, 2020

firefoxci-taskcluster bot commented Oct 4, 2020

Mugurell left a comment

sijanr left a comment

Mugurell commented Oct 6, 2020

sijanr left a comment

Mugurell commented Oct 7, 2020

Mugurell commented Oct 8, 2020

bors bot commented Oct 8, 2020

Mugurell commented Oct 8, 2020

sijanr left a comment

Mugurell commented Oct 9, 2020

bors bot commented Oct 9, 2020

Mugurell left a comment

Mugurell commented Oct 9, 2020

bors bot commented Oct 9, 2020

Mugurell commented Oct 9, 2020

codecov-io commented Oct 9, 2020

For #15402: Hide ETP pop-up if the toolbar is not visible #15667

For #15402: Hide ETP pop-up if the toolbar is not visible #15667

Conversation

sijanr commented Oct 4, 2020

Pull Request checklist

To download an APK when reviewing a PR:

firefoxci-taskcluster bot commented Oct 4, 2020

Mugurell left a comment

Choose a reason for hiding this comment

sijanr left a comment

Choose a reason for hiding this comment

Mugurell commented Oct 6, 2020

sijanr left a comment

Choose a reason for hiding this comment

Mugurell commented Oct 7, 2020

Mugurell commented Oct 8, 2020

bors bot commented Oct 8, 2020

try

Mugurell commented Oct 8, 2020

sijanr left a comment

Choose a reason for hiding this comment

Mugurell commented Oct 9, 2020

bors bot commented Oct 9, 2020

try

Mugurell left a comment

Choose a reason for hiding this comment

Mugurell commented Oct 9, 2020

bors bot commented Oct 9, 2020

Mugurell commented Oct 9, 2020

codecov-io commented Oct 9, 2020

Codecov Report