Get rid of embedding completely by Rossmaxx · Pull Request #7837 · LMMS/lmms

Rossmaxx · 2025-04-07T12:04:16Z

I did run a few vsts for 2 mins and things look fine, on linux mint. More testing might be needed.

cc: @tresf @messmerd @bratpeki @DomClark

Rossmaxx · 2025-04-07T13:00:17Z

What I did here is mostly reverting a major chunk of #3786

Rossmaxx · 2025-04-07T13:44:45Z


-FIND_PACKAGE(Qt5 5.9.0 COMPONENTS Core Gui Widgets Xml Svg REQUIRED)
+IF(LMMS_BUILD_LINUX)
+    FIND_PACKAGE(Qt5 REQUIRED COMPONENTS Core Gui Widgets Xml Svg X11Extras REQUIRED)


I would appreciate if we find a way to push the x11extras stuff to the vst cmake scripts

I would appreciate if we find a way to push the x11extras stuff to the vst cmake scripts

If you're removing embedding in this PR, why would you still need them?

@Rossmaxx on second thought, maybe we keep this... QtX11Info seems to mentioned in this migration guide: https://doc.qt.io/qt-6/extras-changes-qt6.html. (Also we can patch this on qt6 branch)

With regards to where this code belongs, we'll likely need this logic for other plugin types (e.g. LV2) so it's most likely in the right place. We could guard it with a LINUX + VST check if we wanted to be more explicit.

x11extras isn't related to the qt5-x11embed submodule we ship right? It should be fine to leave it alone if my understanding is correct.

Rossmaxx · 2025-04-08T08:09:21Z

Self note: i need to tweak the "keep the vst window on top when not embedded" text.

sakertooth

Left an outstanding comment on the x11extras situation, but LGTM. I can actually build LMMS again. This was also long overdue for a removal.

tresf · 2025-04-14T15:38:55Z

I did run a few vsts for 2 mins and things look fine, on linux mint. More testing might be needed.

If you're spearheading this change, you should know EXACTLY how much testing is needed. At a bare minimum:

Ensure software falls-back gracefully when an embedding technique is specified in the config bug not provided.
Test on all platforms that allow embedding (BOTH Linux and Windows is my initial thought here. Mac allows it but I don't think any plugins can currently use it).
Address the issue that I had raised several times now with the icon in the task bar, quoting:

Currently when an app is not embedded, the icon for it is a generic placeholder, which makes it hard to find when looking for it in the Task Manager. If we nuke embedding completely, I'd like to see some effort into helping identify it, if technically possible.

Some criticisms of this PR:

You failed to link/message Update src/3rdparty/qt5-x11embed to latest version #7825 or @michaelgregorius whose work would be superseded by this PR.
This PR fails to link the Discussion about removing embedding Embedding or not? #7716
This PR also removes embedding support completely, full stop -- which is drastically different from proposals to disable it by default or proposals to modify the configuration of the software to build with support but leave the option off by default (a larger coding effort).

So, although this PR is likely in the direction that the project is moving, I'm not sure if it will get my approval in current form without some more due diligence and supporting conversation around this topic.

Rossmaxx · 2025-04-15T12:01:35Z

Sorry for the late reply @tresf was caught up

If you're spearheading this change, you should know EXACTLY how much testing is needed.

I admit I don't know the exact details, that's why I left it vague, and don't really have much time to mess around as of now. I thought I could submit the PR now and get back once I get more clarity on the topic.

This PR fails to link the Discussion about removing embedding

Ahh, sorry. I will update the description.

drastically different from proposals to disable it by default or proposals to modify the configuration of the software to build with support but leave the option off by default

Disabling it by default is one way to go about it, but wouldn't older installs, from the old lmmsrc override it in that case? That's the reason I haven't attempted that way instead.

I'll draft this PR for now, as I'm not clear about the direction i should take this change to myself.

tresf · 2025-04-15T15:34:25Z

Disabling it by default is one way to go about it, but wouldn't older installs, from the old lmmsrc override it in that case? That's the reason I haven't attempted that way instead.

Yes, this was contextualized in my initial reply:

modify the configuration of the software to build with support but leave the option off by default (a larger coding effort).

tresf · 2025-04-15T15:45:33Z

and don't really have much time to mess around as of now.

If the feature is disabled by default, it's less code changes (so less likely to break even more stuff with open PRs) and can even go as far as to provide a warning when devicePixelRatio > 1 to help people trying this feature against issues like #7683.

To be clear, I too want this feature gone, but only because it's broken, not because it's a bad feature. Embedding external plugins is a great idea in principle.

Furthemore, setting the icon may be adjacent to this task, but once embedding is off by default, we have to make sure that the default experience is sane.

bratpeki · 2025-05-08T13:05:36Z

I don't use VSTs so I haven't tested this. However, it seems like @sakertooth approved. I'd just do another review, test from @Rossmaxx and merge. I could test it when I find some VSTs and time, though. Maybe another review from @szeli1 would be in order, if they find time.

Rossmaxx · 2025-05-08T14:57:57Z

It's been a while since I've done lmms work and I don't think I will for some more time. I did run some brief tests at the time of writing the PR and it seems to look good. However, my testing was very short and I ain't confident with it.

Also, there's some comments from tresf which I haven't addressed because I don't really know what's the correct course of action. I have to think about the problem in more detail.

szeli1

I did not find any problems with the code, although I may not be the best one to review it. I have tested vestige on windows msvc 64 bit, and it worked fine.

szeli1 · 2025-05-11T11:11:11Z

-			VstPlugin::createUI( nullptr );
-		}
+
+		VstPlugin::createUI( nullptr );


Suggested change

VstPlugin::createUI( nullptr );

VstPlugin::createUI(nullptr);

szeli1 · 2025-05-11T11:11:27Z

 {
-	if (! EMBED)
+
+	switch( _m.id )


Suggested change

switch( _m.id )

switch (_m.id)

Rossmaxx added 5 commits April 7, 2025 15:14

nuked the x11embed submodule

b6fef20

begone embedding

aa989ad

fix builds

da2af7c

tidied up everything and a few fixes here and there

07c940b

more fixing builds

671467f

fix ci

4279daf

Rossmaxx marked this pull request as ready for review April 7, 2025 13:41

Rossmaxx commented Apr 7, 2025

View reviewed changes

sakertooth approved these changes Apr 13, 2025

View reviewed changes

This was referenced Apr 15, 2025

Add clangd cache to .gitignore #7846

Merged

Update src/3rdparty/qt5-x11embed to latest version #7825

Merged

Rossmaxx marked this pull request as draft April 15, 2025 12:02

szeli1 self-requested a review May 9, 2025 15:04

szeli1 approved these changes May 11, 2025

View reviewed changes

szeli1 reviewed May 11, 2025

View reviewed changes

	VstPlugin::createUI( nullptr );
	VstPlugin::createUI(nullptr);

Uh oh!

Conversation

Rossmaxx commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rossmaxx commented Apr 7, 2025

Uh oh!

Rossmaxx Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

tresf Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

tresf Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakertooth Apr 13, 2025

Choose a reason for hiding this comment

Uh oh!

Rossmaxx commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sakertooth left a comment

Choose a reason for hiding this comment

Uh oh!

tresf commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rossmaxx commented Apr 15, 2025

Uh oh!

tresf commented Apr 15, 2025

Uh oh!

tresf commented Apr 15, 2025

Uh oh!

bratpeki commented May 8, 2025

Uh oh!

Rossmaxx commented May 8, 2025

Uh oh!

szeli1 left a comment

Choose a reason for hiding this comment

Uh oh!

szeli1 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

szeli1 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Rossmaxx commented Apr 7, 2025 •

edited

Loading

tresf Apr 7, 2025 •

edited

Loading

Rossmaxx commented Apr 8, 2025 •

edited

Loading

tresf commented Apr 14, 2025 •

edited

Loading