To make theme size depend on DPI
ClosedPublic

Authored by mykolak on Feb 12 2018, 12:33 PM.

Details

Summary

BUG: 389826

Added logicalDpi property to use for scaling UI elements size in Theme.qml

Diff Detail

Repository
R255 Elisa
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mykolak requested review of this revision.Feb 12 2018, 12:33 PM
mykolak created this revision.

Thanks for your work.
I used to have something like that but based on the Screen.pixelDensity component for qml.
I believe it would be a good idea to have a look at Plasma or even ask guidance to some of the plasma devs.
I have changed to pure pixel size because Screen.pixelDensity is reporting bogus info if the screen is reporting bogus size (for example my TV reports 16x9 cm for its size). I should probably have replaced it by Screen.devicePixelRatio. Could you also try that ?

Thanks for your work.

Happy to help :)

I used to have something like that but based on the Screen.pixelDensity component for qml.

Main problem with Screen.pixelDensity (first patch I've tested) is that's a physical density reported by current screen. But all texts are scaled to logical DPI. So if physical ≠ logical (when physical is reported wrong, or when big screen is far away and logical DPI enlarged on purpose) we got again same problems. Next linked problem — screens with different DPI's. I've tested on two screens one with 142 dpi and second with 102 dpi and logical DPI set to 144. So Elisa makes jumps in sizes of elements when moving it from one screen to another but texts aren't scaled to current screen. So on second screen (102dpi) we got clipping again.

I believe it would be a good idea to have a look at Plasma or even ask guidance to some of the plasma devs.
I have changed to pure pixel size because Screen.pixelDensity is reporting bogus info if the screen is reporting bogus size (for example my TV reports 16x9 cm for its size). I should probably have replaced it by Screen.devicePixelRatio. Could you also try that ?

Yes, you are right. I'll check Plasma. Screen.devicePixelRatio doesn't solve the problem by itself (it's 1 in my case), but it definitely should be taken into account.

Thanks for your work.
I used to

Thanks for your work.

Happy to help :)

I used to have something like that but based on the Screen.pixelDensity component for qml.

Main problem with Screen.pixelDensity (first patch I've tested) is that's a physical density reported by current screen. But all texts are scaled to logical DPI. So if physical ≠ logical (when physical is reported wrong, or when big screen is far away and logical DPI enlarged on purpose) we got again same problems. Next linked problem — screens with different DPI's. I've tested on two screens one with 142 dpi and second with 102 dpi and logical DPI set to 144. So Elisa makes jumps in sizes of elements when moving it from one screen to another but texts aren't scaled to current screen. So on second screen (102dpi) we got clipping again.

I believe it would be a good idea to have a look at Plasma or even ask guidance to some of the plasma devs.
I have changed to pure pixel size because Screen.pixelDensity is reporting bogus info if the screen is reporting bogus size (for example my TV reports 16x9 cm for its size). I should probably have replaced it by Screen.devicePixelRatio. Could you also try that ?

Yes, you are right. I'll check Plasma. Screen.devicePixelRatio doesn't solve the problem by itself (it's 1 in my case), but it definitely should be taken into account.

I am mostly away from keyboard. There is also another work in the task T7530. Thanks again for your work.

asn added a subscriber: asn.Feb 15 2018, 9:03 AM

I have a HiDPI Screen and the interface looks fine for me. The problem I have is that the fonts are rendered incorrectly. I haven't found any solution for that.

My current patch:
https://xor.cryptomilk.org/elisa/elisa-hidpi.patch.txt

Which still results in:
https://xor.cryptomilk.org/elisa/elisa.png

In D10460#206625, @asn wrote:

I have a HiDPI Screen and the interface looks fine for me. The problem I have is that the fonts are rendered incorrectly. I haven't found any solution for that.

My current patch:
https://xor.cryptomilk.org/elisa/elisa-hidpi.patch.txt

Which still results in:
https://xor.cryptomilk.org/elisa/elisa.png

It looks like you have fractional QT_SCALE_FACTOR it looks similar in my case, when dpi dropped to 96 and QT_SCALE_FACTOR=1.5. As fractional scaling isn't promised to be fine, I'm trying to fix case when scaling set to 1 (or other integral value) and dpi is not 96. In my case 144.

asn added a comment.Feb 15 2018, 12:57 PM

My use a scale factor of 1.4 and Fonts have 144 DPI.

asn added a comment.Feb 15 2018, 2:25 PM

The logout screen (ctrl+alt+del) has exactly the same issues as I have in elisa.

I think the interface is fine, even with your changes applied on top of mine everything seems to scal correctly, just the fonts look horrible.

In D10460#207043, @asn wrote:

The logout screen (ctrl+alt+del) has exactly the same issues as I have in elisa.

I think the interface is fine, even with your changes applied on top of mine everything seems to scal correctly, just the fonts look horrible.

I don't think something could be done for fonts with fractional scale (while QuickControls 1 are used). I have also other issues with fractional scale, that's why I don't use it and prefer to set scaling to 1, change DPI and icon sizes. It works fine for me. Try QT_SCALE_FACTOR=1 elisa.

asn added a comment.Feb 15 2018, 4:38 PM

Ok, everything looks relatively correct with quick qt quick controls 2.3. I've just did a quick hack. Here is my complete patchset.

https://xor.cryptomilk.org/elisa/elisa-hidpi-qtquickcontrols2.3.patch

asn added a comment.Feb 15 2018, 5:00 PM

Are you on IRC or Matrix?

In D10460#207145, @asn wrote:

Ok, everything looks relatively correct with quick qt quick controls 2.3. I've just did a quick hack. Here is my complete patchset.

https://xor.cryptomilk.org/elisa/elisa-hidpi-qtquickcontrols2.3.patch

The issue here is that we would not like to require Qt 5.10 (which is controls 2.3) at this time since it is barely available in distros. I've some patches prepared that ports almost everything to controls 2.2, but it's not done yet since there some smaller issues still. Downside is that especially the buttons with their action component cannot be ported yet to controls 2 and remain unchanged. I will post an update once I'm done.

Since you have the full port to controls 2 on your computer, could you test if setting

QApplication::setAttribute(Qt::AA_EnableHighDpiScaling);

also does the trick?

asn added a comment.Feb 15 2018, 6:54 PM

Alexander, wit the elisa-hidpi-qtquickcontrols2.3.patch everything seems to work. So I think the first step is to move to qt quick controls 2. If you can point me to your patch, I can test it with my changes.

In D10460#207217, @asn wrote:

Alexander, wit the elisa-hidpi-qtquickcontrols2.3.patch everything seems to work. So I think the first step is to move to qt quick controls 2. If you can point me to your patch, I can test it with my changes.

I've pushed a branch called "controls2_port" (not "controls2", that is an old one). It would be great if you could test this branch with the changes from this diff if it also works for you. At least the metadata view for the tracks does not scale correctly right now.
Also, for a future version with a full port to controls2, it would be good to know if we can rely on Qt's built-in scaling, so could you please test your port to 2.3 with the setting of "QApplication::setAttribute(Qt::AA_EnableHighDpiScaling);" in the main.cpp file, but without the changes from this diff?

Great, thanks a lot for testing. +1 from me for this patch then

asn added a comment.Feb 16 2018, 6:27 PM

Alexander, I think you should also cherr-pick the two patches from my repo which set pixmap hidpi and use pointSize instead of pixelSize for fonts.

mykolak added a comment.EditedFeb 17 2018, 10:47 AM
In D10460#207155, @asn wrote:

Are you on IRC or Matrix?

Sadly, not yet. Several times tried to check what that Matrix is, but still...

Sorry for silence. Was busy with work.

I've pushed a branch called "controls2_port" (not "controls2", that is an old one). It would be great if you could test this branch with the changes from this diff if it also works for you. At least the metadata view for the tracks does not scale correctly right now.
Also, for a future version with a full port to controls2, it would be good to know if we can rely on Qt's built-in scaling, so could you please test your port to 2.3 with the setting of "QApplication::setAttribute(Qt::AA_EnableHighDpiScaling);" in the main.cpp file, but without the changes from this diff?

This branch has fine font even with fractional scaling. But still it has the same clipping of text/buttons without this patch in my case when scaling set to 1 and DPI to 144.

PS: how controls2_port looks without this patch: https://photos.app.goo.gl/8ZEFwZA23Lhb61Lk1

In D10460#207155, @asn wrote:

Are you on IRC or Matrix?

Sadly, not yet. Several times tried to check what that Matrix is, but still...

Sorry for silence. Was busy with work.

I've pushed a branch called "controls2_port" (not "controls2", that is an old one). It would be great if you could test this branch with the changes from this diff if it also works for you. At least the metadata view for the tracks does not scale correctly right now.
Also, for a future version with a full port to controls2, it would be good to know if we can rely on Qt's built-in scaling, so could you please test your port to 2.3 with the setting of "QApplication::setAttribute(Qt::AA_EnableHighDpiScaling);" in the main.cpp file, but without the changes from this diff?

This branch has fine font even with fractional scaling. But still it has the same clipping of text/buttons without this patch in my case when scaling set to 1 and DPI to 144.

PS: how controls2_port looks without this patch: https://photos.app.goo.gl/8ZEFwZA23Lhb61Lk1

Thanks for testing those patches. I believe the problem with buttons comes from the fact that they are still controls v1. @astippich is it correct ?

Yes, buttons are still controls v1.

In D10460#207919, @asn wrote:

Alexander, I think you should also cherr-pick the two patches from my repo which set pixmap hidpi and use pointSize instead of pixelSize for fonts.

I will likely ask you to open a revision on phabricator when the branch is pushed into master

astippich accepted this revision as: astippich.Feb 21 2018, 6:09 PM

Any objections for merging?

This revision is now accepted and ready to land.Feb 21 2018, 6:09 PM

Nope. I cannot test but if it is improving things, let's go.

This revision was automatically updated to reflect the committed changes.

For some reason the author in the git log is wrong here. @mgallien what did you do last time or can you fix it?

The solution I had applied when I saw the same situation is the following:

  1. git revert <hash of the commit>

git revert 157c9f04f4f99b5591980fe97052682a3ca6963b

  1. git cherry-pick --no-commit 157c9f04f4f99b5591980fe97052682a3ca6963b
  1. git commit -C 157c9f04f4f99b5591980fe97052682a3ca6963b --author 'the new author <author@test.com>'

Then you can push as usual.

@mykolak could you provide me an email address so that I can add a proper author to the commit?

@astippich sure: Mykola Krachkovsky <w01dnick@gmail.com>