<table><tr><td style="">leinir requested review of this revision.<br />leinir added a comment.
</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><div><p>Ping for some of the frameworks peeps... would be great to get this in sooner rather than later :)</p></div></div><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-136092">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">author.cpp:37</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Suggestion: It might make sense to do this caching at the provider level so users don't need to reimplement this.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">A lot of this will probably want some work when we can break up the BC for KF6... but given the timeframe, yes, that of course means giving the actual bits we want to do some thought now :)</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-136087">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">categoriesmodel.cpp:49</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Suggestion: I tend to make roles static since it doesn't change between instances anyway.<br />
Like so:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">static QHash<int, QByteArray> roles{
    { NameRole, "name" },
    ...
};</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Good call indeed, also perhaps a touch easier to read and such :) (also, this is what the QtQuick ItemsModel already does - i'll leave it to the new models, but probably wants proliferating through the codebase)</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-136088">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">categoriesmodel.h:61</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Suggestion: I use <tt style="background: #ebebeb; font-size: 13px;">const std::unique_ptr<Private> d;</tt> these days, it removes the need to explicitly delete the d pointer.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That sounds like something that'll want doing for KF6 as well, yes - i like the consistency, so just leaving it like this for now, but certainly something for the todo list :)</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-136089">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">commentsmodel.h:38</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Like Author, it might be a good idea to use QQmlParserStatus here.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Less critical codepath, but yeah, why not :)</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-136090">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahiemstra</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Button.qml:112</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Humhum, yes... so it needs to be possible to change enabled from outside, but it also needs to be locked to disabled if allowedByKiosk is false... i'm thinking this probably won't be the prettiest thing, but... catch enabled changing, check whether allowedByKiosk is false and force it to false in the case it is, and otherwise ignore it... unless there's a magic trick i'm not aware of, which would be neat ;)</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>davidedmundson, broulik, ahiemstra, anthonyfieroni, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns<br /></div>