D27544: Fix update scenarios with no explicit downloadlink selected

[Dan Leinir Turthra Jensen]

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


  Thanks for those, @ahiemstra, good stuff there :)

INLINE COMMENTS

> ahiemstra wrote in engine.cpp:614
> Code style: & attaches to the name, not the type. (Yes I hate it too).
> 
> There's a few instances of this around.

so nasty... right, thanks for pointing those out, it feels so wrong to type :P

> ahiemstra wrote in engine.cpp:631
> This object will linger until the engine is destroyed, which seems like a suboptimal thing. Maybe better to do:
> 
>   auto question = std::make_unique<Question>(Question::SelectFromListQuestion);
> 
> then it will be automatically cleaned up once you exit the scope.

Hmm... That's something that'll want doing in a fair few places i think (including the documentation for Question) - but yup, might as well start somewhere :) It does mean turning on C++14 for KNewStuff, which... should be fine?

> ahiemstra wrote in engine.cpp:644
> You may want to log something in the else here, at least then it will be possible to figure out why an update is not happening.

That's probably a good idea, yes... It'll commonly be due to the user cancelling the dialog, but if there's no good questionasker registered, it may happen anyway - plus i guess it's just good style anyway :)

> ahiemstra wrote in EntryDetails.qml:101
> That "1" here is a bit mysterious, what does it actually mean?

It's that whole one-indexed list thing that OCS has... But yup, your idea below sounds pretty good.

> ahiemstra wrote in quickitemsmodel.h:154
> Rather than using -1, you could add an enum that defines these values a bit more, like:
> 
>   enum LinkId {
>       AutoDetectLink = -1,
>       FirstItem = 1
>   }
> 
> You can then also expose that to QML to avoid the magical 1 up there.

A good call, less magic numbers are always good. Did feel a little... unpleasant to add those.

REPOSITORY
  R304 KNewStuff

BRANCH
  fix-update (branched from master)

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

To: leinir, #knewstuff, #frameworks, #plasma, ngraham, apol, #discover_software_store
Cc: ahiemstra, alexde, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200317/81e34310/attachment.html>