<table><tr><td style="">broulik updated this revision to Diff 77882.<br />broulik edited the summary of this revision. <a href="https://phabricator.kde.org/transactions/detail/PHID-XACT-DREV-gycpozv7yemb7sw/">(Show Details)</a><br />broulik edited the test plan for this revision. <a href="https://phabricator.kde.org/transactions/detail/PHID-XACT-DREV-flmmej2qv55ewn5/">(Show Details)</a><br />broulik added reviewers: mart, hpereiradacosta.<br />broulik 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/D28116">View Revision</a></tr></table><br /><div><div><ul class="remarkup-list">
<li class="remarkup-list-item">Use pixel metric for checkbox/radio button size and use the spacing. Only use the size from contents for the (here unused) implicit size of the checkindicator.</li>
</ul></div></div><br /><div><strong>CHANGES TO REVISION SUMMARY</strong><div><div style="white-space: pre-wrap; color: #74777D;">`KQuickStyleItem` manages its implicit size internally. Overriding it on the QML side makes this non-deterministic which assignment wins and might cause unexpected re-evaluation of the size causing it to change.<br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Moreover, the check indicator size already includes spacing between checkbox and label on the `QStyle` side, so our additional `spacing` would be added ontop. Furthermore</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);"><br />
Moreover</span>, <span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">the </span>`Check<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Box` `padding` appears to add padding around the control as a whole /and/ between c</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">Indicator` implicit size is based on `sizeFromContents` for `CT_C</span>heck<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">box and label</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">Box`</span>, <span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">further widening the gap between the two.<br />
Finally,</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">which in case of Breeze (but not the Qt built-in styles) already contains some extra padding on the side between checkbox and label.</span> <span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">because</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">Instead</span> of <span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">the right padd</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">using that for layout</span>ing o<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">f the `C</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">ur full c</span>heck<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">I</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">box (i</span>ndicator<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">`</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);"> + label)</span>, <span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">assigning its `height` to its `width` makes it a lot taller than it should be, resulting in large gaps between controls in e.g. a `FormLayout`</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">do what qqc1 did and use `PM_CheckBoxLabelSpacing` for `spacing` and `PM_IndicatorWidth` as size hint for the indicator.<br />
<br />
Also while at it for radio buttons use the appropriate (`PM_RadioButtonLabelSpacing` and `PM_ExclusiveIndicatorWidth`) hints.<br />
<br />
This makes QQC2 `CheckBox` and `RadioButton` layouted pixel-perfect to their QWidget counterparts and also fixes it randomly changing size hints as you switch between pages as demonstrated by the bug report</span>.<div style="padding: 8px 0;">...</div></div></div></div><br /><div><strong>CHANGES TO TEST PLAN</strong><div><div style="white-space: pre-wrap; color: #74777D;">The only downside might be that using a `CheckIndicator` standalone now has a quite small size (just the checkbox you see basically) rather than a "nice delegate like height". But then I don't think this control is being used anywhere, and is probably centered vertically anway. When used in conjunction with `ItemDelegate` (checkable item delegate) I believe the entire delegate becomes clickable to toggle the `CheckBox` so it getting smaller should have no effect.<br />
<br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Before:</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">Also, using those invokables causes slight layout problems when switching between styles at runtime as they cannot be notified to have changed.<br />
<br />
Before Breeze</span><br />
{F8182462}<span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);"><br />
After Breeze:<br />
{F8182494}<br />
<br />
Before Oxygen:<br />
{F8182503}<br />
After Oxygen:<br />
{F8182497}<br />
<br />
Before Windows:</span><br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">After:</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">{F8182502}</span><br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">{F8182463}</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">After Windows:</span><br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">The gap is still ever so slightly wider than in a `QCheckBox` but that appears to be the style taking into account the "empty label" which still might have some font lead in/out.</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">{F8182499}</span><br />
<br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Edit: Just as I was uploading this, I realized that the spacing is now non-existant with styles other than Breeze ... so I'll have to do some more research on how it gets the sizing :(</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">(I noticed that we use an incorrect palette somewhere since those checkboxes and comboboxes are meant to be white in Windows style but that's an issue unrelated to this patch)</span></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R858 Qt Quick Controls 2: Desktop Style</div></div></div><br /><div><strong>CHANGES SINCE LAST UPDATE</strong><div><a href="https://phabricator.kde.org/D28116?vs=77881&id=77882">https://phabricator.kde.org/D28116?vs=77881&id=77882</a></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D28116">https://phabricator.kde.org/D28116</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>org.kde.desktop/CheckBox.qml<br />
org.kde.desktop/CheckIndicator.qml<br />
org.kde.desktop/RadioButton.qml<br />
plugin/kquickstyleitem.cpp</div></div></div><br /><div><strong>To: </strong>broulik, Plasma, mart, hpereiradacosta<br /><strong>Cc: </strong>plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart<br /></div>