Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display properly on a HiDPI screen #4800

Merged
merged 1 commit into from
Feb 9, 2019
Merged

Conversation

hfiguiere
Copy link
Contributor

@hfiguiere hfiguiere commented Jan 28, 2019

With this LMMS display properly on a HiDPI screen

@PhysSong
Copy link
Member

Thank you for the contribution! However, I believe #3814 already addresses what you want to achieve.

@hfiguiere
Copy link
Contributor Author

hfiguiere commented Jan 28, 2019

PR #3814 doesn't work (I actually tested it doesn't work). I suspect setAttribute is done too late. It has to be done before the QCoreApplication constructor.

@PhysSong
Copy link
Member

Does the new patch work properly? I can't test it now.

@hfiguiere
Copy link
Contributor Author

Yes it does. But then it need to be reworked to remove the code that doesn't work. I'll do that later.

@@ -8,7 +8,7 @@ Comment=Music sequencer and synthesizer
Comment[ca]=Producció fàcil de música per a tothom!
Comment[fr]=Production facile de musique pour tout le monde !
Icon=lmms
Exec=env QT_X11_NO_NATIVE_MENUBAR=1 QT_AUTO_SCREEN_SCALE_FACTOR=1 lmms %f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to documentation, setting QT_AUTO_SCREEN​_SCALE_FACTOR=1 is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but not having to set it is better. And this would make thing not work if lmms was launched differently (ie from the command line).

Which is why the previous change might have been considered working.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but not having to set it is better.

That reason does not convince me. We could drop QT_X11_NO_NATIVE_MENUBAR=1, but we do not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my rationale:

HiDPI should work out of the box. Nothing to do for the user. QCoreApplication::setAttribute() didn't work before, it does now with the patch, make the environment variable QT_AUTO_SCREEN​_SCALE_FACTOR=1 is not needed anymore. Adding it in the .desktop is only useful when the .desktop is used, which is not ideal.

FWIW, I submitted similar patches to Muse Sequencer and Rosegarden and they got accepted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should detect whether the screen is HiDPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qt does that for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Qt documentation says setting Qt::AA_EnableHighDpiScaling before constructing QGuiApplication is equivalent to setting QT_AUTO_SCREEN​_SCALE_FACTOR to 1, as @jasp00 said. However, I agree more with #4800 (comment). It'd be definitely better to have one which should always work and drop another fix which works only in some situations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a fix. It is an opt-in feature that may incur some scaling or painting artifacts. See "Migration of Existing Applications" for the real fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but you were ready to have it except it wasn't properly activated. And without that you can't start fixing said artifacts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm moving this conversation to the conversation area, where it really belongs.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

The Qt documentation says setting Qt::AA_EnableHighDpiScaling before constructing QGuiApplication is equivalent to setting QT_AUTO_SCREEN​_SCALE_FACTOR to 1, as @jasp00 said. However, I agree more with #4800 (comment). It'd be definitely better to have one which should always work and drop another fix which works only in some situations.

Agreed.

HiDPI should work out of the box. Nothing to do for the user. QCoreApplication::setAttribute() didn't work before, it does now with the patch, make the environment variable QT_AUTO_SCREEN​_SCALE_FACTOR=1 is not needed anymore. Adding it in the .desktop is only useful when the .desktop is used, which is not ideal.

Agreed, this is how it works on Mac. I'm curious.. will this fix the same issue for Windows clients?

but you were ready to have it except it wasn't properly activated. And without that you can't start fixing said artifacts.

That change was reverted in the AppImage actually. Apparently we (I) forgot to follow suit in the desktop icon. 22124dd

Anyway, I've had to switch to QT_SCREEN_SCALE_FACTORS="eDP-1=1.5;HDMI-1=1.5;" to make LMMS usable on my machine. I'll test out this PR. Also, we need to know how it behaves on various Qt versions so we have recommendations for people if/when this backfires.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

I'm testing Ubuntu 18.04 w/ Qt 5.9 on a Dell XPS 13 and I can't get either technique to change anything on this machine.

I'm only attaching one screenshot because all tests were identical. Note this display requires fractional scaling, it's not 2:1 like a Retina display.

I have a VM in Parallels on a Apple Retina display that suffers from the inconsistent scaling which I've seen people complain about with the AppImage. I'll test that next with both this PR and the QT_AUTO_SCREEN​_SCALE_FACTOR setting.

screenshot from 2019-02-07 21-36-08

@hfiguiere
Copy link
Contributor Author

This is how it works on Fedora 29 with GNOME+Wayland with autoscale, on the XPS 13:

image

Without autoscale:

image

@hfiguiere
Copy link
Contributor Author

hfiguiere commented Feb 8, 2019

About compatibility, if Lmms is built on Qt < 5.6, this won't work, even if run on >= 5.6

@tresf
Copy link
Member

tresf commented Feb 8, 2019

About compatibility, if Lmms is built on Qt < 5.6, this won't work, even if run on >= 5.6

Understood, I'm talking about the possibility of Qt addressing fixes between Qt 5.6 and now.

No matter what I do, I can't get Ubuntu 18.04 to recognize the QT_AUTO_SCREEN_SCALE_FACTOR setting. I'm on Retina now running 2x scaling and my results are identical to your (bad results) regardless of whether or not QT_AUTO_SCREEN_SCALE_FACTOR is set. I had to force scaling in the VM, so this may be hardware problem. Unfortunately I can't test this PR on any of my machines because it offers no visual difference. Forcing QT_SCALE_FACTOR=2 makes most of the text the correct size, but some components are ballooned so large they're unusable.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

An interesting write-up: https://vicrucann.github.io/tutorials/osg-qt-high-dpi/

@tresf
Copy link
Member

tresf commented Feb 8, 2019

@hfiguiere what Qt version are you using?

@tresf
Copy link
Member

tresf commented Feb 8, 2019

So I just stumbled upon this bug report: linuxmint/cinnamon#4902

The issues are very similar to ours. One person recommended this:

export QT_SCALE_FACTOR=1
export QT_AUTO_SCREEN_SCALE_FACTOR=0
export QT_SCREEN_SCALE_FACTORS=2

Strangely, this fixes scaling for my retina display. I have a feeling that newer (Qt > 5.9) versions behave differently but I'll need to do more testing to confirm this.

@hfiguiere
Copy link
Contributor Author

In the Flatpak it is Qt 5.12.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

In the Flatpak it is Qt 5.12.

That may be the difference then. I'll test against 5.12 now.

@hfiguiere
Copy link
Contributor Author

Fedora 29 has 5.11.3 FWIW. And it works the same.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

Upgrading to QT 5.12 does not appear to make a difference on my machines. The above workaround is still the only setting that fixes the scaling on my retina display in VM.

Same for the XPS 13 (fractional scaling).

The good thing is, this PR doesn't make anything worse.

screen shot 2019-02-07 at 11 31 39 pm

@tresf
Copy link
Member

tresf commented Feb 8, 2019

@hfiguiere are you on Wayland or X11?

@hfiguiere
Copy link
Contributor Author

Wayland. It's the default on Fedora.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

Wayland. It's the default on Fedora.

Thought so. Can you see if you experience the same improvements using X11? My XPS requires fractional scaling (e.g. 125%) which I'm starting to believe is not supported in much except Windows. To switch to X11 usually just requires logging out, clicking the gear and selecting X11 or whatever Fedora calls it. AFAIR the gear will only show if you have a password.

My VM is running inside Parallels which is currently incapable of using Wayland. I'd like to know if the display server is a factor.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

I booted Ubuntu 18.04 to X11 natively on the MacBook Pro with Retina (200%) and can confirm that this patch fixes the scaling issue. I can also confirm that it's functionally equivalent to the QT_AUTO_SCREEN_SCALE_FACTOR environmental variable for machines which honor it.

screenshot from 2019-02-08 05-29-06

In regards to the QT_X11_NO_NATIVE_MENUBAR comments above @jasp00 we'd love to remove that if there's a technique to accomplish the same thing using code but at the time of writing that, I could not find another approach.

This PR should be merged.

@tresf tresf added this to the 1.2.0 milestone Feb 8, 2019
@jasp00
Copy link
Member

jasp00 commented Feb 8, 2019

In regards to the QT_X11_NO_NATIVE_MENUBAR comments above @jasp00 we'd love to remove that if there's a technique to accomplish the same thing using code

Do you still want that?

@tresf
Copy link
Member

tresf commented Feb 8, 2019

Do you still want that?

Want what?

@jasp00
Copy link
Member

jasp00 commented Feb 8, 2019

Remove QT_X11_NO_NATIVE_MENUBAR if there's a technique to accomplish the same thing using code.

@tresf
Copy link
Member

tresf commented Feb 8, 2019

Remove QT_X11_NO_NATIVE_MENUBAR if there's a technique to accomplish the same thing using code.

Ideally. It makes our launcher harder to use out of the box with linuxdeployqt. This is no invite to over-engineer anything. The flag works fine, but if there's a simple way to do it without polluting our launcher that would be nice.

@jasp00
Copy link
Member

jasp00 commented Feb 8, 2019

I will make a request after this one is merged.

@tresf
Copy link
Member

tresf commented Feb 9, 2019

Per discussions above:

  • Having this in code (NOT in our launcher) makes the setting more consistent across platforms
  • @hfiguiere tested with Fedora 29/Wayland/Qt 5.11.3
  • @tresf tested with Ubuntu 18.04/X11/Qt 5.12.1
  • @tresf tested with Ubuntu 18.04/X11/Qt 5.9
  • Qt::AA_EnableHighDpiScaling was set improperly. This is best explained in the Qt documentation.

    ... This attribute must be set before QGuiApplication is constructed. This value was added in Qt 5.6.

  • In addition to the above testing, this was tested on two HiDPI systems that did not take advantage of scaling, but the results were identical before and after this PR.

Despite all of this, there was push back when this was set in the AppImage, so we should expect some pushback for edgecases where this setting makes things worse. We'll do our best to reproduce on similar hardware and handle accordingly.

Merging.

@tresf tresf merged commit bbedfa9 into LMMS:stable-1.2 Feb 9, 2019
@hfiguiere hfiguiere deleted the hidpi branch February 9, 2019 05:35
@tresf tresf mentioned this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants