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
