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
