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.

Sign up for GitHub

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

Display properly on a HiDPI screen #4800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmake/linux/lmms.desktop
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Exec=env QT_X11_NO_NATIVE_MENUBAR=1 lmms %f
Terminal=false
Type=Application
Categories=Qt;AudioVideo;Audio;Midi;
Expand Down
4 changes: 3 additions & 1 deletion src/core/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ int main( int argc, char * * argv )
return EXIT_FAILURE;
}
#endif

#if QT_VERSION >= QT_VERSION_CHECK(5, 6, 0)
QCoreApplication::setAttribute(Qt::AA_EnableHighDpiScaling);
#endif
QCoreApplication * app = coreOnly ?
new QCoreApplication( argc, argv ) :
new MainApplication( argc, argv );
Expand Down
5 changes: 0 additions & 5 deletions src/gui/GuiApplication.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,6 @@ GuiApplication* GuiApplication::instance()

GuiApplication::GuiApplication()
{
// enable HiDPI scaling before showing anything (Qt 5.6+ only)
#if (QT_VERSION >= QT_VERSION_CHECK(5, 6, 0))
QApplication::setAttribute(Qt::AA_EnableHighDpiScaling, true);
#endif

// prompt the user to create the LMMS working directory (e.g. ~/Documents/lmms) if it doesn't exist
if ( !ConfigManager::inst()->hasWorkingDir() &&
QMessageBox::question( NULL,
Expand Down