D16776: Better error handling in KNewStuff backend

[Dan Leinir Turthra Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R134:7f6caf385809: Better error handling in KNewStuff backend 
(authored by leinir).

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16776?vs=45355=45398

REVISION DETAIL
  https://phabricator.kde.org/D16776

AFFECTED FILES
  libdiscover/backends/KNSBackend/KNSBackend.cpp
  libdiscover/backends/KNSBackend/KNSBackend.h

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16776: Better error handling in KNewStuff backend

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  Thanks!

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D16776

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16776: Better error handling in KNewStuff backend

[Dan Leinir Turthra Jensen]

leinir updated this revision to Diff 45355.
leinir marked 2 inline comments as done.
leinir added a comment.


  Address @apol's comments

REPOSITORY
  R134 Discover Software Store

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16776?vs=45166=45355

REVISION DETAIL
  https://phabricator.kde.org/D16776

AFFECTED FILES
  libdiscover/backends/KNSBackend/KNSBackend.cpp
  libdiscover/backends/KNSBackend/KNSBackend.h

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16776: Better error handling in KNewStuff backend

[Dan Leinir Turthra Jensen]

leinir marked 2 inline comments as done.
leinir added a comment.


  In D16776#356741 , @apol wrote:
  
  > Where will the `All categories are missing` error fall right now?
  
  
  That is a configuration file error (which is the case, unless there's a 
network fault, which should be reported earlier). There is a corner case where 
the server might be broken and report all categories missing, even if they 
aren't (such as them having been unexported for some reason or another), but we 
can't detect that on the client, since from the perspective of a client, that 
is just the same thing as the category not existing. So, ConfigFileError 
catches that :)

INLINE COMMENTS

> apol wrote in KNSBackend.cpp:247
> Maybe at least qDebug() it?

guessing the "it" is the error message referred to in the comment below? At any 
rate, yes, debugging it out is not a bad idea at all, i'll do that :) (i do 
notice that there's very few qCDebugs going on... this might want fixing, but 
that's a bit outside the scope of this patch, of course)

> apol wrote in KNSBackend.cpp:275
> Are you setting error after passing error? This reads weird. If you want to 
> pass an empty one use `markInvalid({})`.

really only did this because it was how it was done in the old version of the 
function... But yes, it does kind of read weird, i'll swap them around to make 
it read more sensibly

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D16776

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16776: Better error handling in KNewStuff backend

[Aleix Pol Gonzalez]

apol added a comment.


  Where will the `All categories are missing` error fall right now?

INLINE COMMENTS

> KNSBackend.cpp:247
> +QString error = message;
> +bool invalidFile = false;
> +switch(errorCode) {

Maybe at least qDebug() it?

> KNSBackend.cpp:275
> +case KNSCore::ErrorCode::ProviderError:
> +markInvalid(error);
> +error = i18n("Invalid %1 backend, contact your distributor.", 
> m_displayName);

Are you setting error after passing error? This reads weird. If you want to 
pass an empty one use `markInvalid({})`.

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D16776

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D16776: Better error handling in KNewStuff backend

[Dan Leinir Turthra Jensen]

leinir created this revision.
leinir added reviewers: Discover Software Store, apol.
leinir added a project: Discover Software Store.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
leinir requested review of this revision.

REVISION SUMMARY
  Previously error handling was done using a string matching method, as that 
was all KNewStuffCore offered. A newly modified KNewStuffCore error signal 
(found in D16665 ) gives much better 
opportunities to handle error conditions, and this patch modifies the old 
functionality in Discover's KNewStuff backend to take advantage of this.
  
  BUG: 395937

REPOSITORY
  R134 Discover Software Store

REVISION DETAIL
  https://phabricator.kde.org/D16776

AFFECTED FILES
  libdiscover/backends/KNSBackend/KNSBackend.cpp
  libdiscover/backends/KNSBackend/KNSBackend.h

To: leinir, #discover_software_store, apol
Cc: plasma-devel, masilva, ragreen, ixoos, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart