<table><tr><td style="">leinir marked 12 inline comments as done.<br />leinir added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D21721">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132793">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">atticaprovider.cpp:355</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Since upstream Qt is starting to discourage usage of QList, maybe use QVector instead.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132791">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">atticaprovider.cpp:359</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Generally, it's recommended to use <tt style="background: #ebebeb; font-size: 13px;">std::shared_ptr<KNSCore::Comment> knsComment = std::make_shared<KNSCore::Comment>()</tt>. As a bonus, that allows you to use <tt style="background: #ebebeb; font-size: 13px;">auto knsComment</tt>.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Handy :) One of those places where i'm not opposed to auto, as it's easily read from the right hand side ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132792">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">atticaprovider.cpp:370</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Probably better to use categorised logging for this?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Actually leftovers... but also yes :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132795">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">commentsmodel.cpp:135</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Might want to add a call to checkIndex here (<a href="https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex" class="remarkup-link" target="_blank" rel="noreferrer">https://doc.qt.io/qt-5/qabstractitemmodel.html#checkIndex</a>), it would obsolete some additional checking you do later.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">After confirming that 5.11 (which introduces that function) is indeed acceptable for Frameworks, i'm aaaall over that ;) Very handy little function, that :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132799">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">commentsmodel.cpp:177</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Since you're already checking for <tt style="background: #ebebeb; font-size: 13px;">child</tt> being true (aka not-null), this if becomes rather superfluous.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Hmm... leftovers from earlier versions of that function, there :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132820">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">commentsmodel.h:50</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">CommentsModel { entry: someEntry }</tt> from QML.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132804">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">author.cpp:103</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You should check if the actual property value has changed before changing it. I generally do something along the lines of <tt style="background: #ebebeb; font-size: 13px;">if (newEngine == d->engine) return;</tt>. This helps with preventing binding loops in properties exposed to QML.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Used to do that all the time, not honestly sure why i didn't here. Thanks for the reminder :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132808">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Button.qml:39</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Any particular reason not to alias these directly to the dialog?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132805">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Dialog.qml:34</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If you're using a Qt 5.12 import anyway, might want to unify the other imports to also use .12 versions.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132807">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Dialog.qml:76</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132810">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">DownloadItemsSheet.qml:49</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 ;) )</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132818">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Page.qml:210</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">For readability, it would be better to move these to their own files.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It would, yes... Feared it was a bit clugey, but it was super straightforward, so yay for that ;)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21721#inline-132819">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">qmlplugin.cpp:54</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">engine->setOwnership(instance, QQmlEngine::CppOwnership)</tt> before returning the instance.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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 :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R304 KNewStuff</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21721">https://phabricator.kde.org/D21721</a></div></div><br /><div><strong>To: </strong>leinir, KNewStuff, VDG, Frameworks, ahiemstra<br /><strong>Cc: </strong>ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns<br /></div>