D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:a21bc11fe116: Fix text scaling with non-integer scale 
factors when PLASMA_USE_QT_SCALING=1 is… (authored by ngraham).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11244?vs=29408=29712

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

AFFECTED FILES
  examples/applets/testtheme/contents/ui/FontGizmo.qml
  src/declarativeimports/plasmacomponents/qml/Label.qml
  src/declarativeimports/plasmacomponents/qml/private/DualStateButton.qml
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/Label.qml
  src/declarativeimports/plasmacomponents3/TextArea.qml
  src/declarativeimports/plasmacomponents3/TextField.qml
  src/declarativeimports/plasmastyle/ComboBoxStyle.qml
  src/declarativeimports/plasmastyle/SpinBoxStyle.qml
  src/declarativeimports/plasmastyle/TextAreaStyle.qml
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh, ngraham

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  arcpatch-D11244

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

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh, ngraham

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  Friendly ping!

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh, ngraham

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  So this is weird. I thought this patch fixes those KCMs, because I //can/ get 
them into a fixed state, but it doesn't persist afte4r a reboot. Steps to 
reproduce:
  
  - Start with no scale factor
  - Open System Settings > Display & Monitor > Display > Scale and turn on 1.3x 
scaling
  - Restart system settings (and optionally KWin, to get bigger titlebars
  - Navigate to L KCM. **It looks amazing!* F5752524: L looks amazing after 
restarting system settings, but not the whole machine.png 

  - Reboot the machine
  - Navigate to L KCM again.**Text is once again ugly and pixellated, boo!**
  
  Either way, that's probably a separate issue that I'll have to dig into later.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham updated this revision to Diff 29408.
ngraham added a comment.


  "You missed a spot"

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11244?vs=29366=29408

BRANCH
  arcpatch-D11244

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

AFFECTED FILES
  examples/applets/testtheme/contents/ui/FontGizmo.qml
  src/declarativeimports/plasmacomponents/qml/Label.qml
  src/declarativeimports/plasmacomponents/qml/private/DualStateButton.qml
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/Label.qml
  src/declarativeimports/plasmacomponents3/TextArea.qml
  src/declarativeimports/plasmacomponents3/TextField.qml
  src/declarativeimports/plasmastyle/ComboBoxStyle.qml
  src/declarativeimports/plasmastyle/SpinBoxStyle.qml
  src/declarativeimports/plasmastyle/TextAreaStyle.qml
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Marco Martin]

mart added a comment.


  if they're somewhat related but not risolutive for some of those bugs, use 
CCBUG: instead

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: mart, broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[David Edmundson]

davidedmundson added a comment.


  You've added a load of bug reports about the kcms.
  This patch won't change them at all.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham updated this revision to Diff 29366.
ngraham added a comment.


  Actually fix the problem, using the same fix as in qqc-desktop-style (D11274 
)

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11244?vs=29270=29366

BRANCH
  master

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

AFFECTED FILES
  examples/applets/testtheme/contents/ui/FontGizmo.qml
  src/declarativeimports/plasmacomponents/qml/Label.qml
  src/declarativeimports/plasmacomponents/qml/private/DualStateButton.qml
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/Label.qml
  src/declarativeimports/plasmacomponents3/TextArea.qml
  src/declarativeimports/plasmacomponents3/TextField.qml
  src/declarativeimports/plasmastyle/ComboBoxStyle.qml
  src/declarativeimports/plasmastyle/SpinBoxStyle.qml
  src/declarativeimports/plasmastyle/TextAreaStyle.qml
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  Here's  the bug tracking the QQC2 issue: 
https://bugs.kde.org/show_bug.cgi?id=391780
  
  FWIW, by coincidence someone in the VDG room today actually independently 
noticed the difference between the two rendering styles (In Discover, which 
uses Kirigami, which uses the QQC2 label). You can see it in the following 
image:
  
  F5751079: top Qt bottom Native.jpg 
  
  Qt on the top, Native on the bottom. So yeah, we do want to make sure we're 
always using NativeRendering unless using QtRendering is required to avoid an 
even worse visual issue.
  
  If and when David's Qt patch is merged, we can `#ifdef` this here and in 
QQC2-desktop-style so eventually even people who use non-integer scale factors 
can have the nicest possible text rendering.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham planned changes to this revision.
ngraham added a comment.


  Hah, that's funny. That also implies that there's absolutely nothing wrong 
with `Text.QtRendering`, if QQC2-using non-Plasma apps been accidentally using 
it for years and nobody has found any issues.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[David Edmundson]

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


  Kai's right. The patch doesn't work as indented. You're (accidentally) just 
always enabling Text.QtRendering
  
  If we take your code in Label and change it to the color line
  
color: QtQuickControlsPrivate.Settings.isMobile ||Window.devicePixelRatio % 
1 !== 0 ? "red" : "blue"
  
  It should change depending on QT_SCREEN_SCALE_FACTORS. Yet I always get red.
  It does imply the code in QQC2-desktop-style is wrong too.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  In D11244#223654 , @broulik wrote:
  
  > Did you actually properly test this, ie. without scaling? I can't see how 
this would be working:
  >  1.) You're missing `import QtQuick.Window 2.2`
  >  2.) There's no such property `Window.devicePixelRatio`, it's 
`Screen.devicePixelRatio` (which is also in the `Window` import, but that seems 
also wrong in the `qqc2-desktop-style` you got this from)
  
  
  Yes, I did fully test with:
  
  - No scale factor
  - 1.2x scale factor
  - 2x scale factor
  - `PLASMA_USE_QT_SCALING=1` both on and off for each one
  
  Everything seemed to work fine. `Window.devicePixelRatio` is what 
QQC2-desktop-style used, so I just copied it. If there's something else more 
appropriate, I'm happy to use that instead, or fix the imports, or whatever. 
Again, this patch //does// work--though maybe by accident?
  
  In D11244#223663 , @davidedmundson 
wrote:
  
  > Thanks for finding the bug report.
  >
  > +1 from me.
  >
  > You prompted me to look https://codereview.qt-project.org/222827; turns out 
it was a ridiculously easy fix.
  >
  > Has the advantage that it'll fix the QQC1 desktop theme labels; but I'm ok 
with an interim fix.
  
  
  Wow, fantastic! I look forward to the day when we can remove all of this 
code, then. :)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[David Edmundson]

davidedmundson added a comment.


  Thanks for finding bug report.
  
  +1 from me.
  
  You prompted me to look https://codereview.qt-project.org/222827  turns out 
it was a ridiculously easy fix.
  
  Has the advantage that it'll fix the QQC1 desktop theme labels; but I'm ok 
with an interim fix.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Kai Uwe Broulik]

broulik added a comment.


  Did you actually properly test this, ie. without scaling? I can't see how 
this would be working:
  1.) You're missing `import QtQuick.Window 2.2`
  2.) There's no such property `Window.devicePixelRatio`, it's 
`Screen.devicePixelRatio` (which is also in the `Window` import, but that seems 
also wrong in the `qqc2-desktop-style` you got this from)

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: broulik, #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham updated this revision to Diff 29270.
ngraham added a comment.


  Add the Qt bug we're working around into the coments

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11244?vs=29268=29270

BRANCH
  master

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

AFFECTED FILES
  examples/applets/testtheme/contents/ui/FontGizmo.qml
  src/declarativeimports/plasmacomponents/qml/Label.qml
  src/declarativeimports/plasmacomponents/qml/private/DualStateButton.qml
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/Label.qml
  src/declarativeimports/plasmacomponents3/TextArea.qml
  src/declarativeimports/plasmacomponents3/TextField.qml
  src/declarativeimports/plasmastyle/ComboBoxStyle.qml
  src/declarativeimports/plasmastyle/SpinBoxStyle.qml
  src/declarativeimports/plasmastyle/TextAreaStyle.qml
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  The closest I could find was https://bugreports.qt.io/browse/QTBUG-42606, 
which Kai filed for this exact issue, but only for 2x scale factors. It was 
closed as Cannot Reproduce. I have filed a new one: 
https://bugreports.qt.io/browse/QTBUG-67007

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham added a comment.


  The original workaround from QQC2-desktop-style (which I copied here) doesn't 
have that, sadly. I'm looking for it...

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[David Edmundson]

davidedmundson added a comment.


  Working round a Qt bug always needs a Qt bug report referenced in a code 
comment.

REPOSITORY
  R242 Plasma Framework (Library)

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

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh

D11244: Fix text scaling with non-integer scale factors when PLASMA_USE_QT_SCALING=1 is set

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Plasma, davidedmundson.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
ngraham requested review of this revision.

REVISION SUMMARY
  When `PLASMA_USE_QT_SCALING=1` is set, Plasma uses native Qt scaling. This 
works fine for integer scale factors, and fixes a lot of bugs (see Bug 356446 
 but it introduces a new one: with 
non-integer scale factors, text becomes blurry and pixellated, because 
NativeRenderins is used, and PlasmaComponents QML objects don't implement the 
fix that QQC2-desktop-style does to force the use of Qt font rendering for 
non-integer scale factors. This patch fixes that, and gets us one step closer 
to being able to use Qt scaling in Plasmashell.
  
  There is no effect when `PLASMA_USE_QT_SCALING=1` is not being used.
  
  FIXED-IN 5.13
  BUG: 391691

TEST PLAN
  Before: `PLASMA_USE_QT_SCALING=1` set, 1.2 scale factor: Plasma text looks 
awful
  
  After: `PLASMA_USE_QT_SCALING=1` set, 1.2 scale factor: Plasma text looks 
amazing!
  
  Witout both `PLASMA_USE_QT_SCALING=1` and a non-integer scale factor set, 
there is no visual change.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

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

AFFECTED FILES
  examples/applets/testtheme/contents/ui/FontGizmo.qml
  src/declarativeimports/plasmacomponents/qml/Label.qml
  src/declarativeimports/plasmacomponents/qml/private/DualStateButton.qml
  src/declarativeimports/plasmacomponents3/ComboBox.qml
  src/declarativeimports/plasmacomponents3/Label.qml
  src/declarativeimports/plasmacomponents3/TextArea.qml
  src/declarativeimports/plasmacomponents3/TextField.qml
  src/declarativeimports/plasmastyle/ComboBoxStyle.qml
  src/declarativeimports/plasmastyle/SpinBoxStyle.qml
  src/declarativeimports/plasmastyle/TextAreaStyle.qml
  src/declarativeimports/plasmastyle/TextFieldStyle.qml

To: ngraham, #plasma, davidedmundson
Cc: #frameworks, michaelh