Fetch favicon from the root of the website and use Google as a fallback #36
Conversation
* Replace favicon fetching using Google with fetching from the root of the website * Follow up to 3 http redirects for the favicon * Add download favicon from Google as fallback
There was a problem hiding this comment.
QNetworkRequest defines handling of redirects. That behavior should be used over coding a new method of detecting redirection.
http://doc.qt.io/qt-5/qnetworkrequest.html#setMaximumRedirectsAllowed
|
That feature of QNetworkRequest was introduced on the version 5.6 of Qt and using it will mean that we'll have to increase the minimum required version of Qt from 5.2 which currently is, to 5.6. And 5.6 is still not available on some distros such as Debian Jessie, which by the way is what I'm using. |
|
Good point. I didn't know how "old" Qt 5.6 was. |
| @@ -74,6 +74,9 @@ private Q_SLOTS: | |||
| Database* m_database; | |||
| Uuid m_currentUuid; | |||
| QString m_url; | |||
| QUrl redirectUrl; | |||
There was a problem hiding this comment.
These new member variables do not conform to the coding style. Member variables should be preceded with m_ to differentiate from local/stack variables.
| @@ -141,30 +141,42 @@ void EditWidgetIcons::setUrl(const QString &url) | |||
| abortFaviconDownload(); | |||
| } | |||
|
|
|||
| void EditWidgetIcons::downloadFavicon() | |||
| void EditWidgetIcons::downloadFavicon(QUrl url) | |||
There was a problem hiding this comment.
After reviewing the flow of the code I do not like how this function is used in very different contexts. For example, on first use (no url provided) it kicks off the favicon and redirection code block. On subsequent uses it either follows a redirect or uses the Google URL provided as a last resort. The main (and only) purpose of this function should be to kickstart the download process. Different functions (private) should be defined to handle the different behavior. Or just bake into the onRequestFinished call since it is just a single line of code.
…wnload process. Move code responsible for fetching the favicon from Google on its own function to reduce repetitiveness.
|
Sorry for the last commit but I've noticed it after pushing, so I couldn't combine the commits together. |
|
Excellent rewrite, the flow is MUCH clearer now when you read the code blocks. Thanks! |
Modify the work initially done in keepassxreboot#36, and most recently modified in keepassxreboot#1786, to use DuckDuckGo's https://icons.duckduckgo.com/ip3/www.example.com.ico favicon endpoint. Fixes keepassxreboot#2258
Modify the work initially done in keepassxreboot#36, and most recently modified in keepassxreboot#1786, to use DuckDuckGo's https://icons.duckduckgo.com/ip3/www.example.com.ico favicon endpoint. Fixes keepassxreboot#2258
Modify the work initially done in keepassxreboot#36, and most recently modified in keepassxreboot#1786, to use DuckDuckGo's https://icons.duckduckgo.com/ip3/www.example.com.ico favicon endpoint. Fixes keepassxreboot#2258
…er (#2309) * Replace Google with DuckDuckGo for optional fallback favicon fetch URL Modify the work initially done in #36, and most recently modified in #1786, to use DuckDuckGo's https://icons.duckduckgo.com/ip3/www.example.com.ico favicon endpoint. Fixes #2258 * Close failed favicon fetch progress bars Name the UrlFetchProgressDialog() with the corresponding URL in order to be identified by name by its parent when the failed request is handeled in EditWidgetIcons::fetchFinished(). fetchFinished() retrieves the relevant UrlFetchProgressDialog() and calls close() on it. Fixes: #2265
The way currently the favicon downloading works, doesn't work with some websites. #32