D27083: [PC2]Give Label integer width.

[David Edmundson]

davidedmundson added a comment.


  > In my opinion the Plasma Controls need fix
  
  I would approve that.
  
  Though let's be sure to focus on components3.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: ngraham, davidedmundson, kde-frameworks-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
michaelh, ZrenBot, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[Nathaniel Graham]

ngraham added a comment.


  Probably `frameworks-plasma | components`

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: ngraham, davidedmundson, kde-frameworks-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
michaelh, ZrenBot, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  In D27083#614285 , @ngraham wrote:
  
  > So the test needs to be fixed?
  
  
  In my opinion the Plasma Controls need fix. They need to always display the 
icons in an real position, no matter what their x position is. Just like qt 
controls.
  Using Layout is a way to workaround the problem.
  
  I closed this patch because it was wrong solution, with the intention to open 
a bug report(with a link to this patch). I have yet to find the proper category.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: ngraham, davidedmundson, kde-frameworks-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
michaelh, ZrenBot, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[Nathaniel Graham]

ngraham added a comment.


  So the test needs to be fixed?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: ngraham, davidedmundson, kde-frameworks-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
michaelh, ZrenBot, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo abandoned this revision.
gvgeo added a comment.


  The problem is with controls, not with label. QtQuick controls work fine.
  
  problem
  F8112471: Screenshot_20200219_124506.png 
 F8112472: Screenshot_20200219_124602.png 

  
  Proper icon display with label width real. (PC2 with this patch)
  F8112469: Screenshot_20200219_124730.png 
 F8112470: Screenshot_20200219_125928.png 

  
  F8112477: test.qml 

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  > Also, why is only PC2 affected?
  
  Just checked again, apparently made a mistake. All the labels are affected.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  Should I close this patch then?
  The only place I've seen this problem is in tests.
  I don't know if there is any place that is affected.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  I see, so paintedHeight is not exactly perfect above.
  
  > But I don't really understand how this helps anything in a reliable way.
  >  Layouts will override widths after a layout invalidation. Correctly set 
anchors would override it.
  
  RowLayout, FlowLayout and GridLayout will indeed correct label to integer.
  
  But Row, Flow and Grid do not affect the width, and place the next item with 
fractional x position. The problem exist in more tests, but is easier to see in 
checkboxes.
  
  > Also, why is only PC2 affected?
  
  QtQuick 2 gives label integer size. While QtQuick 1 does not.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[David Edmundson]

davidedmundson added a comment.


  From a logical viewpoirnt it doesn't make sense. It's a binding loop. 
Changing the width will re-lay out, which will change the paintedWidth, which 
will change the width.
  Practically QQuickTextItem has a catch against that (see qquicktext.cpp: 
internalWidthUpdate)  and it will stop after the first pass without a warning.
  
  But I don't really understand how this helps anything in a reliable way.
  Layouts will override widths after a layout invalidation. Correctly set 
anchors would override it.
  It's almost always wrong for a component to set it's own size (as opposed to 
it's implict size) as the consumer of the API will inevitably change it
  
  Also, why is only PC2 affected?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  @davidedmundson ping

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  While I was improving the test.qml, found that Headings also have the same 
problem but vertically. I imagine that is from the fractional-part in the 
multiplier.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, 
ngraham, bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  The Checkbox takes fractional position, caused from the fractional width of 
the Label. This lead to blurriness.
  F8071229: Screenshot_20200201_100353_2.png 

  I don't understand what the problem is. Height can also take the 
`Math.ceil(paintedHeight)` part.
  Got confused, what is the requested change?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, 
bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[David Edmundson]

davidedmundson added a comment.


  I don't understand from the screenshot what changed wrt widths in your 
screenshots. Could you expand on what the bug identified is?

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, 
bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  The math.ceil changes. Absolutely.
  
  Binding width to painted width doesn't make sense. They're different things. 
It'll break any alignment

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, 
bruns, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo added a comment.


  PC3 label gives integer size.
  If this patch is right, will make a patch to change all the tests to use 
PC2/PC3 label, instead of QQC2 for proper display.
  
  The 2 vertical misalignment problems are not the focus of this patch, created 
by:
  
  1. PC2 label height is way bigger that QQC2.
  2. A bug with the checkbox prevents label to take proper height.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: gvgeo, #plasma, #vdg, ndavis
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27083: [PC2]Give Label integer width.

[George Vogiatzis]

gvgeo created this revision.
gvgeo added reviewers: Plasma, VDG, ndavis.
gvgeo added a project: Plasma.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
gvgeo requested review of this revision.

REVISION SUMMARY
  Label paintedWidth creates alingment issues when used in Grid, Flow.
  Also change height round to ceil, to prevent cropping vertical text.

TEST PLAN
  Run `qmlscene test.qml`
  Before:
  Checkbox vertical lines have aliasing ssues.
  After:
  Proper display for second Checkbox.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  label2 (branched from master)

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

AFFECTED FILES
  src/declarativeimports/plasmacomponents/qml/Label.qml

To: gvgeo, #plasma, #vdg, ndavis
Cc: kde-frameworks-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart