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
