D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

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".

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[David Edmundson]

davidedmundson added a comment.


  Please be sure to run the relevant tests in
  
  tests/components/
  
  with qmlscene
  
  It'll find more than running random applets

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


  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.
  F7918757: Screenshot_20200123_002724.PNG 

  `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.
  F7918911: Screenshot_20200123_003405.PNG 

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

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


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

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Nathaniel Graham]

ngraham added a comment.


  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: F7978647: Screenshot_20200127_112436.PNG 

  
  With patch: F7978654: Screenshot_20200127_112559.PNG 

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

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


  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: F7978647: Screenshot_20200127_112436.PNG 

  >
  > With patch: F7978654: Screenshot_20200127_112559.PNG 

  >
  > Everything else looked good to me. Fix that, then let's ship it.
  
  
  I'm pretty sure that test is broken too.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


  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.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


  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 

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  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

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  checkbox-radiobutton (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg, ngraham
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:ad625f68f345: Fix the size and pixel alignment of 
checkboxes and radiobuttons (authored by ndavis).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26758?vs=73852&id=74476

REVISION DETAIL
  https://phabricator.kde.org/D26758

AFFECTED FILES
  src/declarativeimports/plasmacomponents3/CheckIndicator.qml
  src/declarativeimports/plasmacomponents3/RadioButton.qml
  src/declarativeimports/plasmacomponents3/RadioIndicator.qml
  src/declarativeimports/plasmastyle/CheckBoxStyle.qml
  src/declarativeimports/plasmastyle/RadioButtonStyle.qml
  src/desktoptheme/breeze/widgets/actionbutton.svg
  src/desktoptheme/breeze/widgets/checkmarks.svg

To: ndavis, #plasma, #vdg, ngraham
Cc: ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[George Vogiatzis]

gvgeo added a comment.


  Plasma theme explorer is fine. The difference is that it uses 
plasmaComponents, while D26271  uses 
plasmaComponents**3**.
  
  About plasmaComponents:
  There is some bug with the checkbox and the Label. 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 from
  https://bugreports.qt.io/browse/QTBUG-70481
  Should be removed or need to stay awhile?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg, ngraham
Cc: gvgeo, ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[George Vogiatzis]

gvgeo added a comment.


  @ndavis Any chance you missed my previous comment?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg, ngraham
Cc: gvgeo, ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[Noah Davis]

ndavis added a comment.


  In D26758#601718 , @gvgeo wrote:
  
  > Plasma theme explorer is fine. The difference is that it uses 
plasmaComponents, while D26271  uses 
plasmaComponents**3**.
  >
  > 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?
  
  > 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.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg, ngraham
Cc: gvgeo, ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns

D26758: Fix the size and pixel alignment of checkboxes and radiobuttons

[George Vogiatzis]

gvgeo added a comment.


  F8071207: alignment.png 
  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.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D26758

To: ndavis, #plasma, #vdg, ngraham
Cc: gvgeo, ngraham, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, 
michaelh, bruns