[detach-window] Use eventFilter to process closeEvent #8096

sqrvrt · 2025-10-18T15:24:44Z

Finishing PR to #3532.

TODO:

Fix editor windows sometimes defaulting to minimal height available
Enable global shortcuts
EffectView size restrictions
Figure out what's up with LADSPA controls
(partial) Fix One-InstrumentTrackWindow mode
Detach all visible windows immediately after the setting is applied

Probably subject to follow-up PRs:

Globalize more shortcuts (spacebar on any window to play, etc.)
Track subwindow focus independently of whether they're detached

Issues:

Equalizer doesn't collapse when detached
Embedded VST windows detach without a window frame (workaround: detach is disabled)
ITW doesn't accept input after a knob inside it is touched

This frees us from having to inherit Detachable{Widget,Window}, and instead we hook the handling directly in SubWindow.

sqrvrt · 2025-10-18T15:28:12Z

One detail I'm not really sure about is if DetachableWidget should still be called that. It's essentially an eventFilter class that we hook, the "Widget" part might be confusing.

The reason I kept it initially is because its functionality is very close to original DetachableWidget.

All removed functionality is being done in the event handler which gets installed immediately after widget is given to SubWindow.

- Use QLayout::setSizeConstraint to set fixed size since it responds to layout changes. - Set widget icon directly since it's useful for detaching, and SubWindow inherits it anyway.

sqrvrt · 2025-10-19T01:02:42Z

everything aside the initial commit is mostly cleanup of things I noticed in diff between feature/detach-window and master. Could be a collection of small PRs but I think it's fine as it is, especially considering for now I'm the only one working on the branch and there's mostly polishing left.

Make InstrumentTrackWindow prev/next windows preserve detached state, matching how positions are set. SubWindow::attach() move & resize delays removed since their execution order becomes ambiguous, causing issues with manual movement outside the class.

messmerd · 2025-10-19T23:59:19Z

Haven't looked at the code yet, but I tested it and it seems to work pretty well. The prev/next buttons in the instruments seems to work correctly now, which is awesome.

The only issue I noticed is that when you close an attached window (such as the Song Editor) then reopen it, the size of the window is lost and it resets to a smaller size than the default.

messmerd · 2025-10-20T00:06:57Z

And I still think the detached window's close button behavior should be changed. With the way it is currently, I'm certain people will complain that their window disappeared and they aren't able to open it again.

From what I understand, it's a Wayland issue preventing you from moving the window once it's reattached?

sqrvrt · 2025-10-20T06:29:37Z

From what I understand, it's a Wayland issue preventing you from moving the window once it's reattached?

Yes. For at least Qt5, top-level window positions are always reported as 0-0. To avoid having all windows attach to that corner (especially now when scrollbars are gone), they are simply left in the position they were in before detaching.

However, currently you have to go out of your way to run LMMS AppImage in wayland-only mode, and most xwayland integrations actually do support window positioning. So no real users are going to be affected.

With the way it is currently, I'm certain people will complain that their window disappeared and they aren't able to open it again.

I have such doubts as well, but I'm not sure of best way to solve this. Do you suggest simply attachng windows and avoiding moving them entirely? Or do you want to still move them, but have them remain closed until asked to show again?

sqrvrt · 2025-10-20T06:56:09Z

The only issue I noticed is that when you close an attached window (such as the Song Editor) then reopen it, the size of the window is lost and it resets to a smaller size than the default.

Are we talking several pixels smaller or noticeably smaller? If it's the former, a lot of resizing/moving functions don't take SubWindow decorations into account (title bar & borders), which is usually fine-ish, but can add up. I'll probably try to fix that.

sqrvrt · 2025-10-20T06:58:54Z

One more question about behavior: when an editor is detached, should MainWindow editor activation buttons simply raise it or toggle it? Currently they always try to raise the window because it always counts as unfocused which may be confusing to some.

Make SubWindow::attach() translate and move the child widget accounting for SubWindow decorations (frame size and title bar height).

Introduce m_childGeom to track window movements independantly of Qt. Workaround for Qt not moving top-level windows if they're hidden. Additionally make attach & detach preserve the visibility state instead of force-showing the window.

szeli1

I tested this PR and it works great. I remember somebody else making a similar PR.

src/gui/SampleTrackWindow.cpp

src/gui/SubWindow.cpp

include/SubWindow.h

src/gui/SubWindow.cpp

sqrvrt · 2025-10-21T23:12:41Z

I remember somebody else making a similar PR.

if you're talking about #3532, this is a PR to that branch, because the branch is in the LMMS repo and I don't have write access to it (nor should I).

sqrvrt · 2025-10-22T15:10:41Z

Reserving from doing any doxygen inline comments for now for reasons discussed above and since I'm currently unable to generate doxygen to check how it looks for myself.

There's no consistent way to control whether detached windows are resizable. The fixed size needs to be applied directly to child widget. isResizable() being false by default also encourages new effects to have fixed size, which something LMMS is movng away from.

this is already being done in setVisible

sqrvrt · 2025-11-04T16:15:49Z

Note: StereoEnhancer window is way too small and can't even show the title. With the inclusion of the detach button it also becomes hard to move by grabbing title bar.. The controls could probably be inlined directly into EffectView.

sqrvrt · 2025-11-04T16:20:53Z

@messmerd this is probably ready to test & merge now if the build succeeds.

messmerd · 2025-11-04T17:44:46Z

include/EffectControlDialog.h

-	virtual bool isResizable() const { return false; }
-


I don't like that this has been removed. It feels like a step backwards since now plugin devs need to remember to call setSizeConstraint.

I don't really understand the problem that led to you removing isResizable, but is there any other solution?

The problem is that like in other cases, the way this was done without the patch was applying size constraints to the SubWindow. Because detached widgets are no longer contained within a subwindow, they can be resized freely.

The reason isResizable() can't be applied to the child widget is because the child can basically be whatever it wants as long as that is a widget, including but not limited to:

Not having a layout and calling setFixedSize(), even before this commit

Having a layout and calling setFixedSize() with a different size compared to what layout wants (StereoMatrix)

As a result of the above, removing isResizable is mostly just a standardization of sorts - of 23 native effects (not counting LV2 and VST since I can't test them), only 7 were dependent on isResizable()! All others were either resizable by themselves or fixed their size explicitly already.

It feels like a step backwards since now plugin devs need to remember to call setSizeConstraint.

I'd argue for the reverse, most LMMS-native plugins nowadays aren't just solely knobs and have something doing GUI stuff already. And being resizable unless told otherwise is the default behavior of Qt, so I don't see any reason to stray from it.

As an alternative, we could just lie and set isResizable(true) on StereoMatrix since it's the only widget I've seen to break but that's even worse IMO.

Pinging @rubiefawn since she's using isResizable() over at #7805 (6cf9d41), and an alternative would be needed if we go through with removing that method as discussed here.

@rubiefawn actually could you test if the issues #7805 fixes are even present here? As far as I'm concerned, the issue of replacing instruments keeping their size looks to be gone (possibly associated with layout changes to SubWindow), and I wasn't able to reproduce the other issue.

The testing above was performed on the msvc build, but I installed the mingw build and encountered the same behavior there.

While I don't have a msvc or mingw dev environment set up (I do all that on the Linux partition or WSL if I'm desperate) I do still have the installer for the msvc build of 50a73e4 from my initial tests. Reinstalling and testing that shows that actually, ITWs were the only ones that had the controls to begin with.

Compare this to the mingw build of 98e6078:

Other things I noticed:

LADSPA plugins have their layout clipped once detached (at least on the newest commit the scrollbar doesn't also get clipped lmao)

VST plugins have no window border at all once detached

What in the actual... alright, cool. I'm kinda wondering if rebasing this to qt6 would fix this since it's extremely wrong.

...Maybe if I enable FixedSizeDialogHint it starts working?.. I don't know anymore...

So, a couple of observations I have after testing myself.

Apparently Qt::SubWindow isn't mutually exclusive with Qt::Window, that could be the source of some problems.

MSWindowsFixedSizeDialogHint acts as it should, and takes priority over whatever causes no flags to work. If disabling SubWindow flag doesn't work, perhaps CustomizeWindowHint will be the one to go on windows specifically.

That did it!!!

I wonder if that also fixes the MacOS bug

Returns windowing buttons to all ITWs. Also conforms to Qt's advice, see https://doc.qt.io/qt-6/qt.html#WindowType-enum

Could fix some bugs. Also get rid of manipulating Qt::Widget flag manipulation since it's just 0 (fallback).

sqrvrt · 2025-11-08T21:11:28Z

Side note: though SubWindow icons are visible, most of them are very hard to read depending on the OS considering they're white and MSWindows title background is the same shade of white :P

sqrvrt · 2025-11-09T15:00:44Z

So how do we want to handle project save & load? Do we save it with all windows attached? Do we preserve the state and positions of detached windows?

"Ensure both c-s-z and c-y are redo" is removed for parity reasons because it relied on methods that aren't QAction and thus remain local.

sqrvrt · 2025-11-09T23:52:51Z

Regarding shortcuts, should only Undo/Redo be global, or should things like Quit, Save, etc. be too?

sqrvrt · 2025-11-11T12:16:45Z

~~Something that's still an issue is that widening Song editor for some reason doesn't show new patterns~~ Master bug actually

sqrvrt · 2025-11-13T23:31:20Z

One-ITW now partially works; partially in the sense that it doesn't update detached state of new subwindows like arrow keys do, but works in a sense that it actually keeps a single copy of ITW.

Playing notes from keyboard on detached subwindows now partially works, but detached window focus is never updated after it's been detached. ~~This is probably something that needs to be fixed before the merge now.~~ this needs big changes to work.

sqrvrt · 2025-11-15T23:50:04Z

Couldn't get detach-everything to work from the SetupDialog, probably best for it to be manual for now considering the combo boxes can be scrolled "accidentally", so it's not the wisest idea to inflict such a drastic change.

messmerd

Still good

Use eventFilter to process closeEvent

0e7048e

This frees us from having to inherit Detachable{Widget,Window}, and instead we hook the handling directly in SubWindow.

sqrvrt changed the title ~~Use eventFilter to process closeEvent~~ [detach-window] Use eventFilter to process closeEvent Oct 18, 2025

sqrvrt mentioned this pull request Oct 18, 2025

[detach-window] Wrap detached widget into a container #8091

Closed

3 tasks

sqrvrt added 4 commits October 19, 2025 01:18

revert MixerView height cap

76da997

Remove obsolete part of ITW::closeEvent

6fb9f9a

All removed functionality is being done in the event handler which gets installed immediately after widget is given to SubWindow.

refactor SampleTrackWindow, sync detached icon

0fe643f

- Use QLayout::setSizeConstraint to set fixed size since it responds to layout changes. - Set widget icon directly since it's useful for detaching, and SubWindow inherits it anyway.

Set window icon directly on InstrumenTrackView

c1e9c1e

sqrvrt added 2 commits October 19, 2025 22:36

remove leftover comment

806d964

szeli1 self-requested a review October 19, 2025 21:24

sqrvrt added 6 commits October 20, 2025 13:17

Account for size of decorations

7656b1c

Make SubWindow::attach() translate and move the child widget accounting for SubWindow decorations (frame size and title bar height).

Fix ITW movement again (lazy version)

a7f6c60

Move the close hook into SubWindow eventFilter

e3c854b

Keep track of detached window while it's hidden

fac6160

Introduce m_childGeom to track window movements independantly of Qt. Workaround for Qt not moving top-level windows if they're hidden. Additionally make attach & detach preserve the visibility state instead of force-showing the window.

Code cleanup

bad5f04

re-fix ITW because apparently it still breaks

6e45b96

bratpeki self-assigned this Oct 21, 2025

szeli1 reviewed Oct 21, 2025

View reviewed changes

fix codestyle

34681b2

sqrvrt added 2 commits October 22, 2025 22:34

make another if inline

5529ed3

more codestyle fixes

50f70ac

sqrvrt added 3 commits November 4, 2025 18:44

Fix EffectControlDialog not hiding the title bar

f0d57d1

Remove redundant widget->show() on showEvent

50a73e4

this is already being done in setVisible

messmerd reviewed Nov 4, 2025

View reviewed changes

rubiefawn mentioned this pull request Nov 6, 2025

Fix microtuning warning text widening instruments #7805

Merged

rubiefawn linked an issue Nov 6, 2025 that may be closed by this pull request

Restore instrument window size #7876

Open

sqrvrt added 3 commits November 7, 2025 01:45

fix drag&drop not updating ITW SubWindow

e50247e

Disable FixedSizeDialogHint on ITW

98e6078

Returns windowing buttons to all ITWs. Also conforms to Qt's advice, see https://doc.qt.io/qt-6/qt.html#WindowType-enum

Disable SubWindow flag when detaching a window

1a7f6be

Could fix some bugs. Also get rid of manipulating Qt::Widget flag manipulation since it's just 0 (fallback).

Try disabling MacOS soft-fail

49817e4

Enable Undo/Redo globally

0ca0474

"Ensure both c-s-z and c-y are redo" is removed for parity reasons because it relied on methods that aren't QAction and thus remain local.

sqrvrt added 2 commits November 11, 2025 04:09

Save detached window visibility properly; refactor

84e83e0

remove obsolete qt version workarounds

944328f

sqrvrt added 3 commits November 13, 2025 18:57

remove setFixedSize from LadspaControlDialog

029a0e4

Fix resizing LadspaMatrixControlDialog

40d3420

Allow detached windows in topLevelITW; refactor

6aa4b13

sqrvrt force-pushed the detachwindow/event-filter branch from d050008 to 6aa4b13 Compare November 13, 2025 23:23

sqrvrt added 3 commits November 16, 2025 00:37

allow detach to be disabled

17157a0

disable detach on embedded VST subwindows

fcb8fdb

add attach/detach everything actions

d74cd8d

messmerd approved these changes Nov 21, 2025

View reviewed changes

messmerd merged commit ce0bb02 into LMMS:feature/detach-window Nov 21, 2025
10 checks passed

Uh oh!

[detach-window] Use eventFilter to process closeEvent #8096

[detach-window] Use eventFilter to process closeEvent #8096

Uh oh!

Conversation

sqrvrt commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqrvrt commented Oct 18, 2025

Uh oh!

sqrvrt commented Oct 19, 2025

Uh oh!

messmerd commented Oct 19, 2025

Uh oh!

messmerd commented Oct 20, 2025

Uh oh!

sqrvrt commented Oct 20, 2025

Uh oh!

sqrvrt commented Oct 20, 2025

Uh oh!

sqrvrt commented Oct 20, 2025

Uh oh!

szeli1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sqrvrt commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqrvrt commented Oct 22, 2025

Uh oh!

sqrvrt commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqrvrt commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sqrvrt commented Nov 8, 2025

Uh oh!

sqrvrt commented Nov 9, 2025

Uh oh!

sqrvrt commented Nov 9, 2025

Uh oh!

sqrvrt commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqrvrt commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sqrvrt commented Nov 15, 2025

Uh oh!

messmerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

sqrvrt commented Oct 18, 2025 •

edited

Loading

sqrvrt commented Oct 21, 2025 •

edited

Loading

sqrvrt commented Nov 4, 2025 •

edited

Loading

sqrvrt commented Nov 11, 2025 •

edited

Loading

sqrvrt commented Nov 13, 2025 •

edited

Loading