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

[Arjen Hiemstra]

ahiemstra requested changes to this revision.
ahiemstra added a comment.
This revision now requires changes to proceed.


  I have probably missed stuff, but here are my initial comments.

INLINE COMMENTS

> atticaprovider.cpp:355
> +
> +QList<std::shared_ptr<KNSCore::Comment>> getCommentsList(const Attica::Comment::List &comments, std::shared_ptr<KNSCore::Comment> parent) {
> +    QList<std::shared_ptr<KNSCore::Comment>> knsComments;

Since upstream Qt is starting to discourage usage of QList, maybe use QVector instead.

> atticaprovider.cpp:359
> +        qDebug() << "Appending comment with id" << comment.id() << ", which has" << comment.childCount() << "children";
> +        std::shared_ptr<KNSCore::Comment> knsComment(new KNSCore::Comment());
> +        knsComment->id = comment.id();

Generally, it's recommended to use `std::shared_ptr<KNSCore::Comment> knsComment = std::make_shared<KNSCore::Comment>()`. As a bonus, that allows you to use `auto knsComment`.

> atticaprovider.cpp:370
> +        if (comment.childCount() > 0) {
> +            qDebug() << "Getting more comments, as this one has children, and we currently have this number of comments:" << knsComments.count();
> +            knsComments << getCommentsList(comment.children(), knsComment);

Probably better to use categorised logging for this?

> atticaprovider.cpp:410
> +
> +    std::shared_ptr<KNSCore::Author> author(new KNSCore::Author);
> +    author->setId(job->property("username").toString()); // This is a touch hack-like, but it ensures we actually have the data in case it is not returned by the server

Like above, std::make_shared<KNSCore::Author>

> commentsmodel.cpp:135
> +{
> +    QVariant value;
> +    if (index.isValid() && index.row() < d->comments.count()) {

Might want to add a call to checkIndex here (https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex), it would obsolete some additional checking you do later.

> commentsmodel.cpp:177
> +                            ++depth;
> +                            if (child->parent) {
> +                                child = child->parent;

Since you're already checking for `child` being true (aka not-null), this if becomes rather superfluous.

> commentsmodel.h:50
> + *
> + * This model should preferably be constructed by asking the Engine to give a model
> + * instance to you for a specific entry using the commentsForEntry function. If you

Considering this is intended to be used from QML, I would drop this, especially since the current code in engine does nothing special. And changing this would allow for doing `CommentsModel { entry: someEntry }` from QML.

> author.cpp:103
> +{
> +    d->engine = qobject_cast<Engine*>(newEngine);
> +    d->resetConnections();

You should check if the actual property value has changed before changing it. I generally do something along the lines of `if (newEngine == d->engine) return;`. This helps with preventing binding loops in properties exposed to QML.

> Button.qml:39
> +     */
> +    property string configFile: ghnsDialog.configFile
> +

Any particular reason not to alias these directly to the dialog?

> Dialog.qml:34
> +import QtQuick.Dialogs 1.3 as QtDialogs
> +import QtQuick.Layouts 1.12 as QtLayouts
> +import org.kde.newstuff 1.1 as NewStuff

If you're using a Qt 5.12 import anyway, might want to unify the other imports to also use .12 versions.

> Dialog.qml:76
> +     */
> +    signal dialogFinished(var changedEntries);
> +

Any particular reason for using a signal parameter here instead of just setting a property on the dialog? Using a property generally allows for more declarative use of the item.

> DownloadItemsSheet.qml:49
> +        }
> +        Kirigami.AbstractCard {
> +            QtLayouts.Layout.leftMargin: Kirigami.Units.iconSizes.smallMedium

Design wise, I don't think having a card inside an overlaysheet looks too nice. I'm also not sure if the extra attention is needed, so maybe just use a label?

> Page.qml:210
> +
> +    Component {
> +        id: bigPreviewDelegate

For readability, it would be better to move these to their own files.

> qmlplugin.cpp:54
> +        Q_UNUSED(scriptEngine)
> +        return KNewStuffQuick::QuickQuestionListener::instance();
> +    });

Note that this makes the qml engine take ownvership of your singleton instance and it will try to delete it when the engine gets deleted. You probably want to do `engine->setOwnership(instance, QQmlEngine::CppOwnership)` before returning the instance.

REPOSITORY
  R304 KNewStuff

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

To: leinir, #knewstuff, #vdg, #frameworks, ahiemstra
Cc: 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/20190828/479910a7/attachment-0001.html>