<table><tr><td style="">ervin marked 15 inline comments as done.<br />ervin 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/D27840">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/D27840#inline-161124">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateBinding.qml:58</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This description didn't really help me in understanding what it does until I read the code further down.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Went for something much more verbose, but at least it says what it does.</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/D27840#inline-161120">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateBinding.qml:75</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Can you explain this.<br />
Can't you</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);">indicatorComponent.createObject(target, {
settingsState: settingsState
});</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Good point.</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/D27840#inline-161121">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateBinding.qml:91</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">We could also bind <tt style="background: #ebebeb; font-size: 13px;">parent: target</tt> instead?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Tried it and QQuickItem was very angry at me, I didn't push further. :-)</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/D27840#inline-161122">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateBinding.qml:97</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Please check if this works with right-to-left languages (run with <tt style="background: #ebebeb; font-size: 13px;">-reverse</tt> argument to test)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Again that's one of those comments I'd have appreciated on the widgets version a little while ago. ;-)</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/D27840#inline-161117">View Inline</a><span style="color: #4b4d51; font-weight: bold;">davidedmundson</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateIndicator.qml:30</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">This is a faux-button.</p>
<p style="padding: 0; margin: 8px;">Which means it needs all the relevant:<br />
Accessible.blah</p>
<p style="padding: 0; margin: 8px;">roles to be manually added</p>
<p style="padding: 0; margin: 8px;">Should it be in the tab focus chain?</p>
<p style="padding: 0; margin: 8px;">(or we could just inherit ToolButton)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Note this is the case for the QtWidgets implementation as well. I switched to ToolButton here but I likely need to do something similar in my other patch... would have been nice to have this comment earlier there (it's been ready a bit longer), you guys have a bias against widgets based patches. ;-)</p>
<p style="padding: 0; margin: 8px;">I think it shouldn't temper with focus at all though, this is almost impossible to get correct in that context I think, so it'll simply reject focus (not too much of an issue in that context IMHO).</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/D27840#inline-161128">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateIndicator.qml:40</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Perhaps bind to <tt style="background: #ebebeb; font-size: 13px;">icon.valid</tt>?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Done completely differently with ToolButton anyway.</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/D27840#inline-161129">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">SettingStateIndicator.qml:48</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You could make the <tt style="background: #ebebeb; font-size: 13px;">root</tt> <tt style="background: #ebebeb; font-size: 13px;">Item</tt> a <tt style="background: #ebebeb; font-size: 13px;">MouseArea</tt> instead.<br />
Also, I think this should get some kind of hover and/or pressed indication?<br />
Also, what about <tt style="background: #ebebeb; font-size: 13px;">Accessible</tt> and keyboard focus? Should this be a proper <tt style="background: #ebebeb; font-size: 13px;">ToolButton</tt> control instead?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">See my reply to <a href="https://phabricator.kde.org/p/davidedmundson/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@davidedmundson</a> above, I switched to ToolButton.</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/D27840#inline-161131">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">settingstatebindingprivate.cpp:30</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Odd. Wouldn't we just get <tt style="background: #ebebeb; font-size: 13px;">QQuickRowLayout</tt> et al? Or is that if you do a custom item with a <tt style="background: #ebebeb; font-size: 13px;">Layout</tt> as a base?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Or if you make something like Kirigami.FormLayout which has Item as base... obviously it's mostly an heuristic so can't be 100% perfect.</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/D27840#inline-161134">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">settingstatebindingprivate.cpp:82</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Too bad <tt style="background: #ebebeb; font-size: 13px;">QQuickItemChangeListener</tt> is private.</p>
<p style="padding: 0; margin: 8px;">Also, does any of this need event compression?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">That really didn't feel like it required event compression to be honest at least I couldn't perceive any visible impact.</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/D27840#inline-161135">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">settingstatebindingprivate.cpp:90</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Check if it actually changed before emitting</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Well it will have changed in 90% of the cases and checking means doing the computation anyway, which will happen when the getter is called. We'd pay the price twice.</p>
<p style="padding: 0; margin: 8px;">An option is of course to cache the result of the getter to the price of higher complexity. Again I didn't perceive any visible impact on use. Should I go for it anyway?</p>
<p style="padding: 0; margin: 8px;">Note I'm not at all denying this implementation is in some way inefficient, I'm just pointing out the trade off in term of readability and maintainability of the system.</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/D27840#inline-161136">View Inline</a><span style="color: #4b4d51; font-weight: bold;">broulik</span> wrote in <span style="color: #4b4d51; font-weight: bold;">settingstatebindingprivate.cpp:107</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Check if it actually changed before emitting</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Same answer than the previous such case.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R296 KDeclarative</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27840">https://phabricator.kde.org/D27840</a></div></div><br /><div><strong>To: </strong>ervin, crossi, hchain, meven, bport, davidedmundson, mart, ngraham, Frameworks, Plasma<br /><strong>Cc: </strong>broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns<br /></div>