Feature: Detach window #3532
Feature: Detach window #3532
Conversation
lukas-w
force-pushed
the
feature/detach-window
branch
from
016df50 to
cfed76a
Compare
|
Wow! That's really awesome! |
|
Very useful functionality especially when using multi-monitor setup! 👍 I played a little bit with it and found some bugs:
LMMS 1.2.0-rc2.153 |
|
@karmux Thanks for testing! I'll have a look at the issues you pointed out as soon as I find the time. I already noticed number 2 (X-button doesn't work), but couldn't find the cause. |
|
This is super useful!! I can't seem to make a windows build of it though. The build seems to go without a hitch, but when I open the app, it doesn't have the detach button. This might be my own error, but maybe someone else should look into it. I can confirm all the issues that @karmux pointed out. Also, getting the detached window back into lmms is a little unwieldy. I basically need to re-open the window (for exmaple, clicking the FX-Mixer button to re-attach the FX-Mixer). Not exactly a bug, but it would be useful if the minimize button reattached the window, or if there was another button entirely. Good job, looking forward to be able to use this for major producing :) |
|
Sorry for the bump. PS: Sorry to bother... |
|
@NickAcPT Yeah, my idea is that since the minimize button reattaches, they wouldn't have to add another button. The minimize button on the main LMMS window should probably shrink all child windows so there is a way to minimize them. |
Now we've, so I think we can continue working on this.
It's already fixed.
I guess the event is propagated to |
|
Found some more issues:
|
If the minimize button is used for attaching, wouldn't it be confusing? Also, I can't find platform-independent way to add such a button to the title bar unless we create a custom window class which would be quite difficult. There's an easier workaround, wrapping window content and window controls in a layout. It might be a little bit ugly, but it's easy. @lukas-w may I continue this work? |
Go for it. 👍 |
PhysSong
force-pushed
the
feature/detach-window
branch
from
cfed76a to
0b5eae1
Compare
|
Fixed almost all bugs reported by @karmux. As a side effects, minimize button doesn't re-attach windows anymore. I can restore the behavior, but I think that might be confusing. |
I think that's correct. IIRC I copied some code from instrument track and it had some issue(fixed in this PR). |
It never did. This was just suggested by @Jousboxx in #3532 (comment). |
|
hello, |
|
@pwepwe973 This feature is in development and not a part of released versions. Sorry. |
|
thank you
thank you for your message |
|
@PhysSong Can you please summarize the current state of this? Is it ready to be merged? I'm willing to help get this finished if possible. |
I unintentionally abandoned this one, but I can restart working on this.
Not yet, mainly due to #3532 (comment). |
|
I tested the Linux AppImage that was automatically built for this PR, and there were some issues. The detach button didn't display correctly and the taskbar icon bug was back. However, when I built it locally, these issues were gone. I'm using Qt 5.15.2 locally, while the CI uses Qt 5.12.8, so they were likely upstream Qt bugs that got fixed. #7316 updates our Qt version on Linux to Qt 5.15.2, so it would probably fix the CI issues here. I've just been waiting on people to review that PR. |
|
@messmerd Since mixer can't be resized vertically anymore, shouldn't we disable maximizing? Is it even a good idea to disable vertical resizing? |
|
Didn't we add resize ability for mixer recently? Why you saying it ain't resizeable? |
|
It was added back in bf922af#diff-ae4b38c15f268a0cd930edea9a6481663edf21a1ff206291784fd55b8513904eR182 for some reason. This was written long before I joined the discussion so I didn't really question it lol So should we revert that line? |
|
I haven't been following the recent work on the Mixer. If the Mixer in this PR deviates from the resize behavior on master, it should probably be reverted. |
Conflicts: - include/SubWindow.h - src/gui/ControllerRackView.cpp - src/gui/Editor.cpp - src/gui/instrument/InstrumentTrackWindow.cpp NOTE: PR #7514 caused a regression in InstrumentTrackWindow when merged into this detachable windows PR.
|
Also what's the desired behavior for VeSTige controller window? rn it's very janky, doesn't have a title nor a maximize button and its maximum width is less than embedded area width. Should I unfix the width so that it can be maximized and resized? (Here it could also be beneficial to tamper with scrollarea to snap the controls into the empty space, but this is really out of scope now) |
|
#7688 fixes everything remaining I know of. If no new issues are found, after its merge this pr will be actually ready this time. |
|
Another issue I found with this is that opening any track in piano roll snaps it back to main window. This is probably due to the fact it "reopens" when the track is opened and this triggers the snap-back behavior buttons are supposed to do. Something tells me reopening really shouldn't snap windows back. |
|
Is there even a point in differentiation between But |
* Attach windows on close instead of show * use dynamic_cast instead of C cast
|
@SpomJ
They are mostly the same, so if one of them could be removed, that would be awesome. |
Conflicts: - include/ControllerRackView.h - include/Editor.h - include/MixerView.h - src/gui/SampleTrackWindow.cpp - src/gui/SubWindow.cpp - src/gui/editors/Editor.cpp - src/gui/widgets/SimpleTextFloat.cpp
|
Heard some people say detaching straight up doesn't work on MacOS. If that's the case, perhaps we should add a check for mac in |
|
btw working on #8091 right now, if there are any concerns about code style/structure I'd be happy to know. It's apparently a near-half rewrite of the way this currently works (it seems I overestimated the size of this PR lol), so I'm afraid to leave redundant functionality somewhere. |
|
IMO it would also be interesting to have a setting to make all windows detached by default. What do you think? |
|
Can someone explain |
* Use eventFilter to process closeEvent
This frees us from having to inherit Detachable{Widget,Window}, and
instead we hook the handling directly in SubWindow.
* revert MixerView height cap
* Remove obsolete part of ITW::closeEvent
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
- 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
* Preserve detached state in ITW prev/next buttons
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.
* remove leftover comment
* Account for size of decorations
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)
* Move the close hook into SubWindow eventFilter
* Keep track of detached window while it's hidden
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
* re-fix ITW because apparently it still breaks
* fix codestyle
* make another `if` inline
* more codestyle fixes
* Expand SubWindow::setWidget() docs
* Constrain attached window positions
Limit the positions to which the windows can be attached to those that
leave at least a small margin (currently 40px width, 40px height incl.
decorations) of the window visible. This is done to avoid windows
attaching extremely far if they're on a different workspace / monitor,
as well as hopefully help the user avoid losing their windows in
general.
* Hide windows when attaching on closeEvent
* Clarify wayland move workaround
* Remove redundant includes
* make detached closeEvent transparent
* fix editor windows minimizing their height on close
* Enable minimize button on detached windows
* Change detach button SVG
* rename detach icon
* clean up detach.svg
* Disable broken detach for MacOS builds
* fix typo
* revert SimpleTextFloat to master branch
* clean up diff against master
* Fix effect control views bing killed on close
DeleteOnClose flag is unset from being default in SubWindow since it
doesn't seem to be a desired default and in many places simply requires
workarounds that disable it.
Useless size restrictions EffectView imposes on SubWindow are removed
since those are already implicit and conform to layout size policy of
controlView.
* Use, init and update childGeomm only when detached
Tracking is only required for OS-level windows, and so is only
initialized when necessary. Avoids a function overload.
* Use std::clamp
* Make hide-on-attach-on-close configurable
Provide an option to hide/show SubWindow after attaching with close
button.
* Fix window size resetting for real this time
The reason it happened was because SubWindowgot shown before the widget
was visible, causing its height to reset to 0. Now widget visible state
is always set first, so this doesn't happen.
* Restore EffectView functionality actually
* Fix SubWindow label overlapping detach button
* Add SubWindow::eventFilter docs
* Get rid of childGeom
* Keep attached widget position
* Initial attempt at always-detach
* close autodetached windows properly
* Improve window positioning on attach
* Fix editor windows having minimal height on first show
* Remove redundant subwindow-related code from MainWindow
* fix typo in setup menu
* Fix ITW icon always showing up as piano
InstrumentView always overrides the icon (which didn't happen here for
wharever reason), rendering the icon setting useless even without the
bugfix.
* Update ControllerView
* minor formatting fixes
* even more minor formatting fixes
* Don't hard-set size of EffectView
This is optimistic and assumes all widgets that don't have a layout
already fix their size. Even when this isn't true, it's better to set
it inside the effects and not the EffectView.
* Change label text
* Destroy childless subwindows
This is done in hopes that nothing ever uses SubWindow::setWidget
directly. Perhaps a more stable solution using SubWindow::childEvent is
due.
* Get rid of isResizable in EffectView
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.
* Fix EffectControlDialog not hiding the title bar
* Remove redundant widget->show() on showEvent
this is already being done in setVisible
* fix drag&drop not updating ITW SubWindow
* Disable FixedSizeDialogHint on ITW
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
Could fix some bugs. Also get rid of manipulating Qt::Widget flag
manipulation since it's just 0 (fallback).
* Try disabling MacOS soft-fail
* Enable Undo/Redo globally
"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.
* Save detached window visibility properly; refactor
* remove obsolete qt version workarounds
* remove setFixedSize from LadspaControlDialog
* Fix resizing LadspaMatrixControlDialog
* Allow detached windows in topLevelITW; refactor
* allow detach to be disabled
* disable detach on embedded VST subwindows
* add attach/detach everything actions
Conflicts: - src/gui/MainWindow.cpp
|
details**Edit:**To clarify, it looks like showing a TextFloat from a hidden detached window crashes the X11 server in its entirety, and when running wayland-native softlocks the GUI (though strace still shows some activity). Steps to reproduce:
On XWayland it appears to show an empty TextFloat before it dies Edit 2: It wasn't actually a Textfloat itself, but rather the subsequent |
15 participants
Allows detaching a window from LMMS's main window, making things like working on multiple screens easier.
Closes #1259
Quick demonstration:
