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

Wed Aug 28 16:01:25 BST 2019

leinir marked 12 inline comments as done.
leinir added inline comments.

INLINE COMMENTS

> ahiemstra wrote in atticaprovider.cpp:355
> Since upstream Qt is starting to discourage usage of QList, maybe use QVector instead.

That'd be good, except the rest of the KNewStuff API is all QList based. It'll want doing for KF6, but since QList is being deprecated for that anyway, i'm thinking we'll end up with a general QList->QVector porting effort for that time anyway, and right now it'd just be introducing a different API style for seemingly no reason... Otherwise yes :)

> ahiemstra wrote in atticaprovider.cpp:359
> 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`.

Handy :) One of those places where i'm not opposed to auto, as it's easily read from the right hand side ;)

> ahiemstra wrote in atticaprovider.cpp:370
> Probably better to use categorised logging for this?

Actually leftovers... but also yes :)

> ahiemstra wrote in commentsmodel.cpp:135
> 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.

After confirming that 5.11 (which introduces that function) is indeed acceptable for Frameworks, i'm aaaall over that ;) Very handy little function, that :)

> ahiemstra wrote in commentsmodel.cpp:177
> Since you're already checking for `child` being true (aka not-null), this if becomes rather superfluous.

Hmm... leftovers from earlier versions of that function, there :)

> ahiemstra wrote in commentsmodel.h:50
> 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.

It's not really intended to be used from QML, i really only left out a more clever wrapper because... well, it's not really that heavy. But, prompted by this, i have in fact produced that abstraction, so there is now a NewStuff.CommentsModel, which makes things a bit easier during implementation, and more delarativey ;)

> ahiemstra wrote in author.cpp:103
> 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.

Used to do that all the time, not honestly sure why i didn't here. Thanks for the reminder :)

> ahiemstra wrote in Button.qml:39
> Any particular reason not to alias these directly to the dialog?

Yes, if it's aliased directly, we end up initialising the engine as soon as the Button component is instantiated, which we want to avoid (as it's quite an expensive operation, and causes internet traffic the user might very well have no intention of using... which arguably is also a phone-home situation, and that needs to be avoided). I'll make an implementationy comment about that. (not really documentation relevant, but certainly relevant when someone invariably comes across this in the future and asks the same question)

> ahiemstra wrote in Dialog.qml:34
> If you're using a Qt 5.12 import anyway, might want to unify the other imports to also use .12 versions.

Don't actually need 5.12 ones... so i'll scale back to 5.11, but also a bunch of cleanup elsewhere, and also actually /require/ 5.11 because of that other bit of handy functionality with checkIndex :)

> ahiemstra wrote in Dialog.qml:76
> 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.

Because a changedEntries property would logically be for the lifetime of the component instance, where dialogFinished is for this specific time the dialog was opened... The property would reasonably also be useful, but it would be semantically different (unless it's documented as being cleared when the dialog is shown, and then filled with whatever's changed once the dialog has been closed... which we could do, but kind of feels uglier than this signal)

> ahiemstra wrote in DownloadItemsSheet.qml:49
> 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?

This definitely is something that'd be great to hear from the VDG about (who i seem to not be able to @ as a group, i guess because it's not really a user or whatnot... certainly would be handy ;) )

> ahiemstra wrote in Page.qml:210
> For readability, it would be better to move these to their own files.

It would, yes... Feared it was a bit clugey, but it was super straightforward, so yay for that ;)

> ahiemstra wrote in qmlplugin.cpp:54
> 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.

Hmm... Quite. It doesn't really matter functionally, but the code in the QuickQuestionListener suggests that it's cpp owned, so yeah, let's just do that :)

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/35708bc2/attachment-0001.html>