[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: [frameworks/knewstuff] src/core: KNSCoree::Engine: Use QUrl for reading providerFileUrl
From: Ben Cooksley <bcooksley () kde ! org>
Date: 2023-02-09 7:32:29
Message-ID: CA+XidOF=SC=1HAowpPdj+eeMy-6bhYCdy_Zr3366BLqxN7yKiw () mail ! gmail ! com
[Download RAW message or body]
Hi Alexander,
With regards to the below change to KNewStuff has it been rigorously tested
to ensure that this change does not impact on how it communicates with and
behaves with server-side infrastructure?
I can appreciate that it looks fairly safe and harmless, however i've been
burned too many times by QNetworkAccessManager and it's associated classes
not to ask that we explicitly test and check that the behaviour remains
correct.
Thanks,
Ben
On Thu, Feb 9, 2023 at 8:14 AM Alexander Lohnau <null@kde.org> wrote:
> Git commit 44f327eee36a1065cc4415d8f412b734609b7d00 by Alexander Lohnau.
> Committed on 06/02/2023 at 20:49.
> Pushed by alex into branch 'master'.
>
> KNSCoree::Engine: Use QUrl for reading providerFileUrl
>
> M +9 -10 src/core/engine.cpp [INFRASTRUCTURE]
>
>
> https://invent.kde.org/frameworks/knewstuff/commit/44f327eee36a1065cc4415d8f412b734609b7d00
>
> diff --git a/src/core/engine.cpp b/src/core/engine.cpp
> index 90982272..0bf92662 100644
> --- a/src/core/engine.cpp
> +++ b/src/core/engine.cpp
> @@ -54,7 +54,7 @@
>
> using namespace KNSCore;
>
> -typedef QHash<QString, XmlLoader *> EngineProviderLoaderHash;
> +typedef QHash<QUrl, XmlLoader *> EngineProviderLoaderHash;
> Q_GLOBAL_STATIC(QThreadStorage<EngineProviderLoaderHash>,
> s_engineProviderLoaders)
>
> class EnginePrivate
> @@ -154,8 +154,7 @@ public:
> QSharedPointer<Cache> cache;
> QTimer *searchTimer = new QTimer();
> // The url of the file containing information about content providers
> - /// TODO KF6 This really wants to be turned into a QUrl (which will
> have implications for our public API, so not doing it just now)
> - QString providerFileUrl;
> + QUrl providerFileUrl;
> // Categories from knsrc file
> QStringList categories;
>
> @@ -272,14 +271,14 @@ bool Engine::init(const QString &configfile)
> d->uploadEnabled = group.readEntry("UploadEnabled", true);
> Q_EMIT uploadEnabledChanged();
>
> - d->providerFileUrl = group.readEntry("ProvidersUrl", QStringLiteral("
> https://autoconfig.kde.org/ocs/providers.xml"));
> - if (d->providerFileUrl == QLatin1String("
> https://download.kde.org/ocs/providers.xml")) {
> - d->providerFileUrl = QStringLiteral("
> https://autoconfig.kde.org/ocs/providers.xml");
> + d->providerFileUrl = group.readEntry("ProvidersUrl",
> QUrl(QStringLiteral("https://autoconfig.kde.org/ocs/providers.xml")));
> + if (d->providerFileUrl.toString() == QLatin1String("
> https://download.kde.org/ocs/providers.xml")) {
> + d->providerFileUrl = QUrl(QStringLiteral("
> https://autoconfig.kde.org/ocs/providers.xml"));
> qCWarning(KNEWSTUFFCORE) << "Please make sure" << configfile <<
> "has ProvidersUrl=https://autoconfig.kde.org/ocs/providers.xml";
> }
> if (group.readEntry("UseLocalProvidersFile", "false").toLower() ==
> QLatin1String{"true"}) {
> // The local providers file is called "appname.providers", to
> match "appname.knsrc"
> - d->providerFileUrl =
> QUrl::fromLocalFile(QLatin1String("%1.providers").arg(configFullPath.left(configFullPath.length()
> - 6))).toString();
> + d->providerFileUrl =
> QUrl::fromLocalFile(QLatin1String("%1.providers").arg(configFullPath.left(configFullPath.length()
> - 6)));
> }
>
> d->tagFilter = group.readEntry("TagFilter",
> QStringList(QStringLiteral("ghns_excluded!=1")));
> @@ -404,7 +403,7 @@ void Engine::loadProviders()
> }
> }
> });
> - loader->load(QUrl(d->providerFileUrl));
> + loader->load(d->providerFileUrl);
> }
> connect(loader, &XmlLoader::signalLoaded, this,
> &Engine::slotProviderFileLoaded);
> connect(loader, &XmlLoader::signalFailed, this,
> &Engine::slotProvidersFailed);
> @@ -425,7 +424,7 @@ void Engine::slotProviderFileLoaded(const QDomDocument
> &doc)
> } else if (providers.tagName() != QLatin1String("ghnsproviders") &&
> providers.tagName() != QLatin1String("knewstuffproviders")) {
> qWarning() << "No document in providers.xml.";
> Q_EMIT signalErrorCode(KNSCore::ProviderError,
> - i18n("Could not load get hot new stuff
> providers from file: %1", d->providerFileUrl),
> + i18n("Could not load get hot new stuff
> providers from file: %1", d->providerFileUrl.toString()),
> d->providerFileUrl);
> return;
> }
> @@ -507,7 +506,7 @@ void Engine::providerJobStarted(KJob *job)
>
> void Engine::slotProvidersFailed()
> {
> - Q_EMIT signalErrorCode(KNSCore::ProviderError, i18n("Loading of
> providers from file: %1 failed", d->providerFileUrl), d->providerFileUrl);
> + Q_EMIT signalErrorCode(KNSCore::ProviderError, i18n("Loading of
> providers from file: %1 failed", d->providerFileUrl.toString()),
> d->providerFileUrl);
> }
>
> void Engine::providerInitialized(Provider *p)
>
>
[Attachment #3 (text/html)]
<div dir="ltr">Hi Alexander,<div><br></div><div>With regards to the below change to \
KNewStuff has it been rigorously tested to ensure that this change does not impact \
on how it communicates with and behaves with server-side \
infrastructure?</div><div><br></div><div>I can appreciate that it looks fairly safe \
and harmless, however i've been burned too many times by QNetworkAccessManager \
and it's associated classes not to ask that we explicitly test and check that the \
behaviour remains correct.</div><div><br></div><div>Thanks,</div><div>Ben</div></div><br><div \
class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 9, 2023 at 8:14 AM \
Alexander Lohnau <<a href="mailto:null@kde.org">null@kde.org</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Git commit \
44f327eee36a1065cc4415d8f412b734609b7d00 by Alexander Lohnau.<br> Committed on \
06/02/2023 at 20:49.<br> Pushed by alex into branch 'master'.<br>
<br>
KNSCoree::Engine: Use QUrl for reading providerFileUrl<br>
<br>
M +9 -10 src/core/engine.cpp [INFRASTRUCTURE]<br>
<br>
<a href="https://invent.kde.org/frameworks/knewstuff/commit/44f327eee36a1065cc4415d8f412b734609b7d00" \
rel="noreferrer" target="_blank">https://invent.kde.org/frameworks/knewstuff/commit/44f327eee36a1065cc4415d8f412b734609b7d00</a><br>
<br>
diff --git a/src/core/engine.cpp b/src/core/engine.cpp<br>
index 90982272..0bf92662 100644<br>
--- a/src/core/engine.cpp<br>
+++ b/src/core/engine.cpp<br>
@@ -54,7 +54,7 @@<br>
<br>
using namespace KNSCore;<br>
<br>
-typedef QHash<QString, XmlLoader *> EngineProviderLoaderHash;<br>
+typedef QHash<QUrl, XmlLoader *> EngineProviderLoaderHash;<br>
Q_GLOBAL_STATIC(QThreadStorage<EngineProviderLoaderHash>, \
s_engineProviderLoaders)<br> <br>
class EnginePrivate<br>
@@ -154,8 +154,7 @@ public:<br>
QSharedPointer<Cache> cache;<br>
QTimer *searchTimer = new QTimer();<br>
// The url of the file containing information about content providers<br>
- /// TODO KF6 This really wants to be turned into a QUrl (which will have \
implications for our public API, so not doing it just now)<br>
- QString providerFileUrl;<br>
+ QUrl providerFileUrl;<br>
// Categories from knsrc file<br>
QStringList categories;<br>
<br>
@@ -272,14 +271,14 @@ bool Engine::init(const QString &configfile)<br>
d->uploadEnabled = group.readEntry("UploadEnabled", true);<br>
Q_EMIT uploadEnabledChanged();<br>
<br>
- d->providerFileUrl = group.readEntry("ProvidersUrl", \
QStringLiteral("<a href="https://autoconfig.kde.org/ocs/providers.xml" \
rel="noreferrer" target="_blank">https://autoconfig.kde.org/ocs/providers.xml</a>"));<br>
- if (d->providerFileUrl == QLatin1String("<a \
href="https://download.kde.org/ocs/providers.xml" rel="noreferrer" \
target="_blank">https://download.kde.org/ocs/providers.xml</a>")) \
{<br>
- d->providerFileUrl = QStringLiteral("<a \
href="https://autoconfig.kde.org/ocs/providers.xml" rel="noreferrer" \
target="_blank">https://autoconfig.kde.org/ocs/providers.xml</a>");<br> + \
d->providerFileUrl = group.readEntry("ProvidersUrl", \
QUrl(QStringLiteral("<a href="https://autoconfig.kde.org/ocs/providers.xml" \
rel="noreferrer" target="_blank">https://autoconfig.kde.org/ocs/providers.xml</a>")));<br>
+ if (d->providerFileUrl.toString() == QLatin1String("<a \
href="https://download.kde.org/ocs/providers.xml" rel="noreferrer" \
target="_blank">https://download.kde.org/ocs/providers.xml</a>")) {<br> + \
d->providerFileUrl = QUrl(QStringLiteral("<a \
href="https://autoconfig.kde.org/ocs/providers.xml" rel="noreferrer" \
target="_blank">https://autoconfig.kde.org/ocs/providers.xml</a>"));<br> \
qCWarning(KNEWSTUFFCORE) << "Please make sure" << configfile \
<< "has ProvidersUrl=<a \
href="https://autoconfig.kde.org/ocs/providers.xml" rel="noreferrer" \
target="_blank">https://autoconfig.kde.org/ocs/providers.xml</a>";<br> }<br>
if (group.readEntry("UseLocalProvidersFile", \
"false").toLower() == QLatin1String{"true"}) \
{<br>
// The local providers file is called "appname.providers", to \
match "appname.knsrc"<br>
- d->providerFileUrl = \
QUrl::fromLocalFile(QLatin1String("%1.providers").arg(configFullPath.left(configFullPath.length() \
- 6))).toString();<br> + d->providerFileUrl = \
QUrl::fromLocalFile(QLatin1String("%1.providers").arg(configFullPath.left(configFullPath.length() \
- 6)));<br> }<br>
<br>
d->tagFilter = group.readEntry("TagFilter", \
QStringList(QStringLiteral("ghns_excluded!=1")));<br> @@ -404,7 +403,7 @@ \
void Engine::loadProviders()<br> }<br>
}<br>
});<br>
- loader->load(QUrl(d->providerFileUrl));<br>
+ loader->load(d->providerFileUrl);<br>
}<br>
connect(loader, &XmlLoader::signalLoaded, this, \
&Engine::slotProviderFileLoaded);<br>
connect(loader, &XmlLoader::signalFailed, this, \
&Engine::slotProvidersFailed);<br> @@ -425,7 +424,7 @@ void \
Engine::slotProviderFileLoaded(const QDomDocument &doc)<br> } else if \
(providers.tagName() != QLatin1String("ghnsproviders") && \
providers.tagName() != QLatin1String("knewstuffproviders")) \
{<br>
qWarning() << "No document in providers.xml.";<br>
Q_EMIT signalErrorCode(KNSCore::ProviderError,<br>
- i18n("Could not load get hot new \
stuff providers from file: %1", d->providerFileUrl),<br> + \
i18n("Could not load get hot new stuff providers from file: %1", \
d->providerFileUrl.toString()),<br>
d->providerFileUrl);<br>
return;<br>
}<br>
@@ -507,7 +506,7 @@ void Engine::providerJobStarted(KJob *job)<br>
<br>
void Engine::slotProvidersFailed()<br>
{<br>
- Q_EMIT signalErrorCode(KNSCore::ProviderError, i18n("Loading of providers \
from file: %1 failed", d->providerFileUrl), d->providerFileUrl);<br> + \
Q_EMIT signalErrorCode(KNSCore::ProviderError, i18n("Loading of providers from \
file: %1 failed", d->providerFileUrl.toString()), d->providerFileUrl);<br> \
}<br> <br>
void Engine::providerInitialized(Provider *p)<br>
<br>
</blockquote></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic