Differential D26758

Fix the size and pixel alignment of checkboxes and radiobuttons
ClosedPublic
Actions

Authored by ndavis on Jan 19 2020, 2:52 AM.

Details

Reviewers
ngraham
Group Reviewers
Plasma
VDG
Commits
R242:ad625f68f345: Fix the size and pixel alignment of checkboxes and radiobuttons
Summary

The checkboxes and radio buttons in plasmashell were signficantly different from checkboxes and radio buttons in the Breeze QStyle. This patch is needed to make D26271 look nicer.

  • Plasmashell checkboxes and radio buttons were much larger.
    • 24x24 for checkboxes and 29x29 for radio buttons (Plasmashell) vs 16x16 (QStyle).
  • Pixel alignment was very poor.
    • Checkbox indicator was 14x14, stretched to 24x24.
    • Radio button background was 22x22 or 32x32, adjusted to 29x29.
    • Radio button indicator was 16x16, stretched to 29x29.
  • Style was different.
    • Checkbox indicators had sharper corners and less margin around the inner rectangle.
    • Radio buttons had a hardcoded dark gray circular frame and a shadow on the bottom of the inside of the frame.
    • Radio button indicators had less margin around the inner circle.
Test Plan

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ndavis created this revision.Jan 19 2020, 2:52 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 19 2020, 2:52 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ndavis requested review of this revision.Jan 19 2020, 2:52 AM
ndavis added a comment.Jan 19 2020, 3:00 AM

This patch has a few problems and some things to consider.

  • Changing the size to units.iconSizes.small doesn't seem to break any layouts we have, but I have not done extensive testing with 3rd party widgets.
  • Is the size too small for touch input? If yes, that would mean the QStyle needs to be changed too.
  • For whatever reason, the checkboxes and radio buttons seem to grow in height and width by maybe 1px when hovered or checked. I can't figure out why and it still causes some pixel alignment issues, particularly with radio buttons.
  • Is it desirable to have multiple sizes for checkboxes and radio buttons? If not, then I suppose I should remove every size other than the one we choose from the svgs. Currently, I have 16, 22, 24 and 32px versions in both svgs.
ndavis edited the summary of this revision. (Show Details)Jan 19 2020, 3:00 AM
ndavis edited the test plan for this revision. (Show Details)
ndavis added a comment.Jan 19 2020, 3:09 AM

For some reason, the corners of the checkbox indicator don't overlap perfectly with the corners of the base checkbox, even though the corners are exactly the same in the SVGs.

ndavis retitled this revision from [WIP] Fix the size and pixel alignment of checkboxes and radiobuttons to Fix the size and pixel alignment of checkboxes and radiobuttons.Jan 22 2020, 8:16 PM
davidedmundson added a subscriber: davidedmundson.Jan 22 2020, 9:25 PM

Please be sure to run the relevant tests in

tests/components/

with qmlscene

It'll find more than running random applets

ndavis added a comment.Jan 23 2020, 5:44 AM
In D26758#599231, @davidedmundson wrote:

Please be sure to run the relevant tests in

tests/components/

with qmlscene

It'll find more than running random applets

Unfortunately, those have visual glitches, even for widgets that are normally perfectly fine.
qmlscene button.qml (untouched by this patch): The position of the left side is off by half a pixel.


qmlscene checkbox.qml (patch vs git master): these unchecked checkboxes are normally perfectly fine, but are somehow very wrong. I'm not sure what's going there.

I can't figure out what might be wrong with the tests just by looking at the source code of the QML files.

ndavis added a comment.Jan 27 2020, 5:32 PM

Ping. This patch doesn't fix everything, but it's still an improvement over the current state.

ngraham added a subscriber: ngraham.Jan 27 2020, 6:27 PM

Found one small regression in the test suite. The new correct radio button sizing results in vertical misalignment (for both PC2 and PC3):

Status quo:

With patch:

Everything else looked good to me. Fix that, then let's ship it.

ndavis added a comment.Jan 28 2020, 1:09 AM
In D26758#601535, @ngraham wrote:

Found one small regression in the test suite. The new correct radio button sizing results in vertical misalignment (for both PC2 and PC3):

Status quo:

With patch:

Everything else looked good to me. Fix that, then let's ship it.

I'm pretty sure that test is broken too.

ndavis added a comment.Jan 28 2020, 1:12 AM

In case it's not clear or for anyone skimming the conversation, I think the tests themselves are broken and can't be relied upon because they break things that normally work OK or at least better than they do in the tests. I don't know QML that well, but I don't see any obvious problems in the code of the tests.

ngraham added a comment.Jan 28 2020, 4:49 AM

I'd like to be able to verify that by looking at a radio button in the Plasma style but I can't actually find any. Do you know of any place where they're used?

ndavis added a comment.Jan 28 2020, 4:54 AM
In D26758#601675, @ngraham wrote:

I'd like to be able to verify that by looking at a radio button in the Plasma style but I can't actually find any. Do you know of any place where they're used?

plasmathemeexplorer, also D26271

ngraham accepted this revision.Jan 28 2020, 5:27 AM

Looks visually broken in the exact same way in Plasma Theme Explorer, so I suspect the test code was lifted from it which explains why it's broken in the test.

Looks perfect in D26271. Shipit!

Then let's go fix the test and Plasma Theme Explorer lol

This revision is now accepted and ready to land.Jan 28 2020, 5:27 AM
Closed by commit R242:ad625f68f345: Fix the size and pixel alignment of checkboxes and radiobuttons (authored by ndavis). · Explain WhyJan 28 2020, 6:31 AM
This revision was automatically updated to reflect the committed changes.
gvgeo added a subscriber: gvgeo.EditedJan 28 2020, 10:01 AM

Plasma theme explorer is fine. The difference is that it uses plasmaComponents, while D26271 uses plasmaComponents3.

About plasmaComponents:
There is some bug with the checkbox and the Label height. Fails to take the correct size (uses paintedHeight).

height: Math.round(Math.max(paintedHeight, theme.mSize(theme.defaultFont).height*1.6))

Radiobutton draws correctly the label height. But need indicator vertical placement or different label height.

~~~~
Non relevant note:
There is a workaround in multiple places for
https://bugreports.qt.io/browse/QTBUG-67007
Which was fixed in Qt 5.14.0 RC1 from
https://bugreports.qt.io/browse/QTBUG-70481
Should be removed or need to stay awhile?

gvgeo added a comment.EditedJan 29 2020, 4:58 PM

@ndavis Any chance you missed my previous comment?

(Pinging cause of the UI regression.)

ndavis added a comment.EditedJan 31 2020, 10:29 PM
In D26758#601718, @gvgeo wrote:

Plasma theme explorer is fine. The difference is that it uses plasmaComponents, while D26271 uses plasmaComponents3.

About plasmaComponents:
There is some bug with the checkbox and the Label height. Fails to take the correct size (uses paintedHeight).

height: Math.round(Math.max(paintedHeight, theme.mSize(theme.defaultFont).height*1.6))

Radiobutton draws correctly the label height. But need indicator vertical placement or different label height.

Sorry, I don't understand what the problem is and I don't know the QML code well. I didn't touch that code (I think), so I'm not sure it has anything to do with this patch. Can you state the problem in simpler terms? What do you see on the screen?

Non relevant note:
There is a workaround in multiple places for
https://bugreports.qt.io/browse/QTBUG-67007
Which was fixed in Qt 5.14.0 RC1 from
https://bugreports.qt.io/browse/QTBUG-70481
Should be removed or need to stay awhile?

I'd suggest removing them when the minimum Qt requirement is raised up to or past that version. If the workarounds cause visual glitches, then ask whoever maintains the affected repos about it.

gvgeo added a comment.Feb 1 2020, 10:03 AM


This shows the problem.
Don't know what is the correct approach:
1 Label needs to be shorter, like PC3.
2 Give correct height in checkbox, and center the buttons.
3 Override label height in radiobutton, to make everything slim. Will need to center when used.

Sorry for the non relevant note, thought you were maintainer. You answered anyway, thanks.

Revision Contents
Changeset List

Diff 74476

View Options

src/declarativeimports/plasmacomponents3/CheckIndicator.qml

Loading...
View Options

src/declarativeimports/plasmacomponents3/RadioButton.qml

Loading...
View Options

src/declarativeimports/plasmacomponents3/RadioIndicator.qml

Loading...
View Options

src/declarativeimports/plasmastyle/CheckBoxStyle.qml

Loading...
View Options

src/declarativeimports/plasmastyle/RadioButtonStyle.qml

Loading...
View Options

src/desktoptheme/breeze/widgets/actionbutton.svg

Loading...
View Options

src/desktoptheme/breeze/widgets/checkmarks.svg

Loading...