Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #463: Fixes resolution issue in the favorites view #611

Merged
merged 2 commits into from Mar 4, 2019

Conversation

Projects
None yet
2 participants
@danielbogomazov
Copy link
Contributor

commented Dec 9, 2018

Pull Request Checklist

  • My patch has gone through review and I have addressed review comments

  • My patch has a standard commit message that looks like Fix #123: This fixes the shattered coffee cup!

  • I have updated the Unit Tests to cover new or changed functionality

  • I have updated the UI Tests to cover new or changed functionality

  • I have marked the bug with [needsuplift]

  • I have made sure that localizable strings use NSLocalizableString()

Screenshots

Notes for testing this patch

Fixes issue 463

The issue stems from passing in a non-nil icon value into the setIcon function which uses that image as an upscale instead of using the FaviconFetcher.

@@ -67,9 +67,10 @@ class FavoritesDataSource: NSObject, UICollectionViewDataSource {
guard let fav = frc?.object(at: indexPath) else { return UICollectionViewCell() }

cell.textLabel.text = fav.displayTitle ?? fav.url
cell.imageView.setIcon(fav.domain?.favicon, forURL: URL(string: fav.url ?? ""), scaledDefaultIconSize: CGSize(width: 40, height: 40), completed: { (color, url) in
let n: FaviconMO? = nil // Distinguishes which setIcon function is called

This comment has been minimized.

Copy link
@jhreis

jhreis Jan 18, 2019

Collaborator

This is kind of an interesting workaround, however, there should be a more determined solution than using a typed optional set to nil to choose the function. One option would be to give the setIcon function different declarations/names, to avoid overloading and calling the wrong function.

@danielbogomazov

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

The function names have been changed to help distinguish them. Let me know if there's anything else to fix :)

@jhreis

jhreis approved these changes Mar 4, 2019

@jhreis jhreis merged commit 1368a45 into brave:development Mar 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jhreis

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2019

Thanks bunches! Really appreciate the work on this, and thanks for your huge patience with us 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.