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

Don't let plugins make the main window transparent #3809

Merged
merged 2 commits into from Sep 25, 2017

Conversation

Projects
5 participants
@DomClark
Member

DomClark commented Sep 15, 2017

Some plugins, e.g. Blue Cat's VSTs, have an opacity control which, in LMMS, makes the main window transparent. This PR blocks this behaviour under Windows by preventing the WS_EX_LAYERED extended window style from being set.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 15, 2017

Member

@DomClark thanks. This looks pretty safe to merge. Does this prevent all transparencies or just those invoked from remote plugins?

Member

tresf commented Sep 15, 2017

@DomClark thanks. This looks pretty safe to merge. Does this prevent all transparencies or just those invoked from remote plugins?

@DomClark

This comment has been minimized.

Show comment
Hide comment
@DomClark

DomClark Sep 15, 2017

Member

It prevents all transparencies achieved with the Windows API which require setting an extended window style, for all windows whose messages are processed inside QCoreApplication::exec(). This includes all remote-plugin-invoked transparencies and excludes all Qt ones, so should be safe to include. Windows doesn't, however, provide a way to find out which process or window is making the modifications, so this could well also prevent third-party applications designed to make windows transparent from doing so to LMMS. I'm not sure if you would consider that a problem or not.

Also, I've made a slight mistake where windows that already have the WS_EX_LAYERED style will lose it if their extended style is modified, I'll fix that up now.

Member

DomClark commented Sep 15, 2017

It prevents all transparencies achieved with the Windows API which require setting an extended window style, for all windows whose messages are processed inside QCoreApplication::exec(). This includes all remote-plugin-invoked transparencies and excludes all Qt ones, so should be safe to include. Windows doesn't, however, provide a way to find out which process or window is making the modifications, so this could well also prevent third-party applications designed to make windows transparent from doing so to LMMS. I'm not sure if you would consider that a problem or not.

Also, I've made a slight mistake where windows that already have the WS_EX_LAYERED style will lose it if their extended style is modified, I'll fix that up now.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 15, 2017

Member

I'm not sure if you would consider that a problem or not.

Not at all. 👍

Member

tresf commented Sep 15, 2017

I'm not sure if you would consider that a problem or not.

Not at all. 👍

@Umcaruje Umcaruje added this to In Progress in Release 1.2.0 RC4 Sep 18, 2017

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Sep 18, 2017

Member

Ok so I tested this on Linux Qt4, and turning the transparency knob has the same behaviour with or without this PR:
gifrecord_2017-09-18_220655

The window basically becomes uninitialized and copies whatever is underneath it, and can't be reset without removing and adding the effect again. So this PR fixes the problem on windows, but not so much on linux.

Member

Umcaruje commented Sep 18, 2017

Ok so I tested this on Linux Qt4, and turning the transparency knob has the same behaviour with or without this PR:
gifrecord_2017-09-18_220655

The window basically becomes uninitialized and copies whatever is underneath it, and can't be reset without removing and adding the effect again. So this PR fixes the problem on windows, but not so much on linux.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 24, 2017

Member

Ok so I tested this on Linux Qt4...

This fix is #ifdef LMMS_BUILD_WIN32
@DomClark Is it possible to fix this for Linux/Wine too?

Merge?

Member

zonkmachine commented Sep 24, 2017

Ok so I tested this on Linux Qt4...

This fix is #ifdef LMMS_BUILD_WIN32
@DomClark Is it possible to fix this for Linux/Wine too?

Merge?

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Sep 24, 2017

Member

FYI #2826 seems to resolve this issue on linux, both on Qt4 and Qt5(if #3786 is merged).

Edit: It just ignores opacity settings in X11embed method, which is used in Qt4 and going to be default in Qt5. QWindow approach always work correctly regardless of #2826. Couldn't test with QWidget::createWindowContainer() method.

Member

PhysSong commented Sep 24, 2017

FYI #2826 seems to resolve this issue on linux, both on Qt4 and Qt5(if #3786 is merged).

Edit: It just ignores opacity settings in X11embed method, which is used in Qt4 and going to be default in Qt5. QWindow approach always work correctly regardless of #2826. Couldn't test with QWidget::createWindowContainer() method.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Sep 24, 2017

Member

FYI #2826 seems to resolve this issue on linux, both on Qt4 and Qt5(if #3786 is merged).

OK. So Linux is fixed elsewhere. Merge this?

Member

zonkmachine commented Sep 24, 2017

FYI #2826 seems to resolve this issue on linux, both on Qt4 and Qt5(if #3786 is merged).

OK. So Linux is fixed elsewhere. Merge this?

@Umcaruje Umcaruje merged commit dd429c5 into LMMS:stable-1.2 Sep 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine moved this from In Progress to Done in Release 1.2.0 RC4 Sep 25, 2017

@DomClark

This comment has been minimized.

Show comment
Hide comment
@DomClark

DomClark Oct 3, 2017

Member

Turns out this only works for Qt4. In Qt5 native event filtering is achieved via QCoreApplication::installNativeEventFilter. I'll implement this once I've got a Qt5 build environment working, unless anyone else wants to do so first.

Member

DomClark commented Oct 3, 2017

Turns out this only works for Qt4. In Qt5 native event filtering is achieved via QCoreApplication::installNativeEventFilter. I'll implement this once I've got a Qt5 build environment working, unless anyone else wants to do so first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment