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.

Sign up for GitHub

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

favicon fetch improvements #1786

Merged
merged 3 commits into from
Apr 5, 2018
Merged

favicon fetch improvements #1786

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bba2043
EditWidgetIcons: remove redundant include
tycho Mar 27, 2018
eefe088
favicons: improve favicon fetching process
tycho Mar 27, 2018
76a6b71
EditWidgetIcons: support older Qt versions without QNetworkRequest::F…
tycho Mar 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions src/gui/EditWidgetIcons.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,21 @@ namespace {
QString newdom = parts.takeLast() + url.topLevelDomain();
return newdom;
}

QUrl convertVariantToUrl(QVariant var)
{
QUrl url;
if (var.canConvert<QUrl>())
url = var.value<QUrl>();
return url;
}

QUrl getRedirectTarget(QNetworkReply *reply)
{
QVariant var = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
QUrl url = convertVariantToUrl(var);
return url;
}
}
#endif

Expand All @@ -183,6 +198,7 @@ void EditWidgetIcons::downloadFavicon()
#ifdef WITH_XC_NETWORKING
m_ui->faviconButton->setDisabled(true);

m_redirects = 0;
m_urlsToTry.clear();

QString fullyQualifiedDomain = m_url.host();
Expand Down Expand Up @@ -219,17 +235,32 @@ void EditWidgetIcons::fetchFinished()
QImage image;
bool googleFallbackEnabled = config()->get("security/IconDownloadFallbackToGoogle", false).toBool();
bool error = (m_reply->error() != QNetworkReply::NoError);
QUrl redirectTarget = getRedirectTarget(m_reply);

m_reply->deleteLater();
m_reply = nullptr;

if (!error) {
image.loadFromData(m_bytesReceived);
if (redirectTarget.isValid()) {
// Redirected, we need to follow it, or fall through if we have
// done too many redirects already.
if (m_redirects < 5) {
m_redirects++;
if (redirectTarget.isRelative())
redirectTarget = m_fetchUrl.resolved(redirectTarget);
startFetchFavicon(redirectTarget);
return;
}
} else {
// No redirect, and we theoretically have some icon data now.
image.loadFromData(m_bytesReceived);
}
}

if (!image.isNull()) {
addCustomIcon(image);
} else if (!m_urlsToTry.empty()) {
m_redirects = 0;
startFetchFavicon(m_urlsToTry.takeFirst());
return;
} else {
Expand Down Expand Up @@ -258,9 +289,9 @@ void EditWidgetIcons::startFetchFavicon(const QUrl& url)
#ifdef WITH_XC_NETWORKING
m_bytesReceived.clear();

m_fetchUrl = url;

QNetworkRequest request(url);
request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, true);
request.setAttribute(QNetworkRequest::RedirectPolicyAttribute, QNetworkRequest::NoLessSafeRedirectPolicy);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to use these attributes if the Qt version is > 5.9? Likewise in the code above this only perform the redirect checks if the Qt version is older.

#if QT_VERSION >= QT_VERSION_CHECK(5, 9, 0)
// Do attribute stuff
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's messier to do the ifdefs. Why not just do it this way until the future when 5.9 is prolific and then move it over to the simpler way?

Copy link
Member

Choose a reason for hiding this comment

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

The ifdefs are messier, but it only exposes a small population to the workaround code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I get the argument. From what I read, you're advocating for:

  1. having the less readable ifdefs,
  2. having multiple possible code paths that are chosen at compile-time,
  3. making it harder to debug if something goes wrong (which code path got used at compile time? which Qt version is it?)

Why would you want that instead of a single well-tested and readable code path that all builds use?

Copy link
Member

Choose a reason for hiding this comment

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

Well when you put it that way!


m_reply = m_netMgr.get(request);
connect(m_reply, &QNetworkReply::finished, this, &EditWidgetIcons::fetchFinished);
Expand Down
2 changes: 2 additions & 0 deletions src/gui/EditWidgetIcons.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,12 @@ private slots:
Uuid m_currentUuid;
#ifdef WITH_XC_NETWORKING
QUrl m_url;
QUrl m_fetchUrl;
QList<QUrl> m_urlsToTry;
QByteArray m_bytesReceived;
QNetworkAccessManager m_netMgr;
QNetworkReply *m_reply;
int m_redirects;
#endif
DefaultIconModel* const m_defaultIconModel;
CustomIconModel* const m_customIconModel;
Expand Down