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

Conversation

@darkwing
Copy link
Contributor

@darkwing darkwing commented Apr 7, 2020

Fixes #6564. We don't want the entire URL, just the hostname, per @topotropic's direction.


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 Apr 7, 2020

Codecov Report

Merging #6566 into master will not change coverage by %.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6566   +/-   ##
=========================================
  Coverage     77.62%   77.62%           
  Complexity     4718     4718           
=========================================
  Files           627      627           
  Lines         22959    22959           
  Branches       3384     3384           
=========================================
  Hits          17823    17823           
  Misses         3746     3746           
  Partials       1390     1390           
Impacted Files Coverage Δ Complexity Δ
...zilla/components/browser/tabstray/TabViewHolder.kt 93.10% <0.00%> (ø) 5.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 2f4dba9...0bd7546. Read the comment docs.

@darkwing darkwing force-pushed the 6564-tabstray-url-hostname branch 2 times, most recently from 09cd495 to 30ab353 Compare April 7, 2020 22:14
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Nice, almost there! We should add tests to add coverage around that try-catch block.

We have a very short guide to generating a coverage report for the component you're working on to see where test coverage is lacking.

You should also have a look at setting up detekt and ktlint, which is used for most of our Android projects, so that it's run before pushing a patch up to CI. This will save personal time and CPU time to do it as a pre-push hook.

Comment on lines 52 to 58

var url = tab.url
try {
url = URL(tab.url).host
}
catch (e: MalformedURLException) { /* ignore error */ }
urlView?.text = url
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the catch clause properly to set the fallback URL if the parsing fails.

Suggested change
var url = tab.url
try {
url = URL(tab.url).host
}
catch (e: MalformedURLException) { /* ignore error */ }
urlView?.text = url
try {
val tabHost = URL(tab.url).host
urlView?.text = tabHost
} catch (e: MalformedURLException) {
urlView?.text = tab.url
}

It's generally not good practice to not handle the exceptions. If this is not the place you want to handle it, you can annotate the as a method that throws exceptions and the caller will then have to put the try-catch clause appropriately

Copy link
Contributor

@csadilek csadilek Apr 8, 2020

Choose a reason for hiding this comment

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

nit: we should lift the assignment out of the try catch e.g:

urlView?.text = try {
    URL(tab.url).host          
} catch (e: MalformedURLException) {
    tab.url
}

Copy link
Contributor

@csadilek csadilek Apr 8, 2020

Choose a reason for hiding this comment

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

We also have an extension function for this we could re-use here:

So, you can just call:

urlView?.text = tab.url.tryGetHostFromUrl()

@jonalmeida
Copy link
Contributor

We also need to add a changelog entry. :)

@darkwing darkwing force-pushed the 6564-tabstray-url-hostname branch from 30ab353 to 134ec7b Compare April 7, 2020 22:49
@jonalmeida jonalmeida self-assigned this Apr 8, 2020
@jonalmeida jonalmeida added the ✏️ needs changes PRs that need some changes/fixes before they can land label Apr 8, 2020
@darkwing darkwing force-pushed the 6564-tabstray-url-hostname branch from 134ec7b to 0bd7546 Compare April 8, 2020 14:14
Copy link
Contributor

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @darkwing !

bors r+

bors bot pushed a commit that referenced this pull request Apr 8, 2020
6566: For #6564 - Only display URL hostname for tabstray r=jonalmeida a=darkwing

Fixes #6564. We don't want the entire URL, just the hostname, per @topotropic's direction.



Co-authored-by: David Walsh <davidwalsh83@gmail.com>
@bors
Copy link

bors bot commented Apr 8, 2020

Build failed

@jonalmeida
Copy link
Contributor

bors retry

@bors
Copy link

bors bot commented Apr 9, 2020

Build succeeded

@bors bors bot merged commit 52ca085 into mozilla-mobile:master Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

✏️ needs changes PRs that need some changes/fixes before they can land

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tabstray items should only display URL hostname, not entire URL

3 participants