D21721: Bring KNewStuffQuick to feature parity with KNewStuff(Widgets)

[Dan Leinir Turthra Jensen]

leinir requested review of this revision.
leinir added a comment.


  Ping for some of the frameworks peeps... would be great to get this in sooner rather than later :)

INLINE COMMENTS

> ahiemstra wrote in author.cpp:37
> Suggestion: It might make sense to do this caching at the provider level so users don't need to reimplement this.

A lot of this will probably want some work when we can break up the BC for KF6... but given the timeframe, yes, that of course means giving the actual bits we want to do some thought now :)

> ahiemstra wrote in categoriesmodel.cpp:49
> Suggestion: I tend to make roles static since it doesn't change between instances anyway.
> Like so:
> 
>   static QHash<int, QByteArray> roles{
>       { NameRole, "name" },
>       ...
>   };

Good call indeed, also perhaps a touch easier to read and such :) (also, this is what the QtQuick ItemsModel already does - i'll leave it to the new models, but probably wants proliferating through the codebase)

> ahiemstra wrote in categoriesmodel.h:61
> Suggestion: I use `const std::unique_ptr<Private> d;` these days, it removes the need to explicitly delete the d pointer.

That sounds like something that'll want doing for KF6 as well, yes - i like the consistency, so just leaving it like this for now, but certainly something for the todo list :)

> ahiemstra wrote in commentsmodel.h:38
> Like Author, it might be a good idea to use QQmlParserStatus here.

Less critical codepath, but yeah, why not :)

> ahiemstra wrote in Button.qml:112
> Hmm, this is quite tricky, as a user of the Button I can now no longer safely bind enabled as that would override the allowedByKiosk binding. You should probably make this an explicit binding so it doesn't break as easily.

Humhum, yes... so it needs to be possible to change enabled from outside, but it also needs to be locked to disabled if allowedByKiosk is false... i'm thinking this probably won't be the prettiest thing, but... catch enabled changing, check whether allowedByKiosk is false and force it to false in the case it is, and otherwise ignore it... unless there's a magic trick i'm not aware of, which would be neat ;)

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190920/033151b2/attachment.html>