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

[Arjen Hiemstra]

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


  I went over it again and found a few more small things and also added some suggestions. Feel free to apply or ignore the suggestions. Once the other items have been taken care of, I think this is good to go.

INLINE COMMENTS

> commentsmodel.cpp:99
> +            int pageToLoad = comments.count() / commentsPerPage;
> +            qDebug() << "Loading comments, page" << pageToLoad << "with current comment count" << comments.count() << "out of a total of" << entry.numberOfComments();
> +            provider->loadComments(entry, commentsPerPage, pageToLoad);

Categorise or drop :)

> engine.h:155
> +     * The sort mode set on the current request
> +     * @see setFilter(Provider::SortMode)
> +     * @since 5.62

I assume you mean setSortMode here?

> author.cpp:37
> +typedef QHash<QString, std::shared_ptr<KNSCore::Author>> AllAuthorsHash;
> +Q_GLOBAL_STATIC(AllAuthorsHash, allAuthors)
> +

Suggestion: It might make sense to do this caching at the provider level so users don't need to reimplement this.

> categoriesmodel.cpp:49
> +{
> +    QHash<int, QByteArray> roles;
> +    roles[NameRole] = "name";

Suggestion: I tend to make roles static since it doesn't change between instances anyway.
Like so:

  static QHash<int, QByteArray> roles{
      { NameRole, "name" },
      ...
  };

> categoriesmodel.h:61
> +    class Private;
> +    Private* d;
> +};

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

> commentsmodel.h:38
> + */
> +class CommentsModel : public QIdentityProxyModel
> +{

Like Author, it might be a good idea to use QQmlParserStatus here.

> Button.qml:112
> +    visible: enabled || visibleWhenDisabled
> +    enabled: ghnsDialog.engine.allowedByKiosk
> +

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.

REPOSITORY
  R304 KNewStuff

BRANCH
  knsquick-feature-parity-with-kns (branched from master)

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/20190918/005b02d7/attachment-0001.html>