Maximize button for resizable instruments #7514

michaelgregorius · 2024-09-20T20:18:50Z

Show the maximize button for resizable instruments.

Most other changes have the character of refactorings and code reorganizations.

Remove the negation in the if condition for resizable instruments to make the code better readable.

Only manipulate the system menu if the instrument is not resizable.

Add a TODO to the special code that sets a size.

Unfortunately there are still some artifacts for some reason, i.e. the title bar still seems to be shown for a maximized instrument window:

Show the maximize button for resizable instruments. Most other changes have the character of refactorings and code reorganizations. Remove the negation in the if condition for resizable instruments to make the code better readable. Only manipulate the system menu if the instrument is not resizable. Add a TODO to the special code that sets a size.

michaelgregorius · 2024-09-21T07:33:36Z

Ok, this one is weird. I have added the following code snippet to MainWindow::finalize right after the for loop that adds the editor windows:

auto wid = new QWidget();
wid->setMinimumSize(QSize(200, 100));
wid->setWindowTitle("Window title");
//wid->setStyleSheet("background-color:#108010;");
addWindowedWidget(wid);

As you can see setting the style sheet is commented out and can be commented back in. Here's how the maximized window looks without a style sheet being set:

The title bar of the window can still be seen for some reason.

Here's the same situation but with the style sheet being set for the new window/widget:

Now there is no title bar and it looks like expected.

I guess the instrument window is in some strange in between state.

Does anybody have an idea what causes this behavior?

michaelgregorius · 2024-09-21T08:02:17Z

This seems to be caused by LMMS' own SubWindow class because it works as expected if I directly add a QMdiSubWindow via the MDI area:

auto wid = new QWidget();
wid->setMinimumSize(QSize(200, 100));
wid->setWindowTitle("Window title");
m_workspace->addSubWindow(wid);

michaelgregorius · 2024-09-22T09:51:09Z

Maximizing also works as expected when the instrument window is added as a "normal" QMdiSubWindow, i.e. not as a SubWindow:

In `SubWindow::paintEvent` don't paint anything if the sub window is maximized . Otherwise some gradients are visible behind the maximized child content. In `SubWindow::adjustTitleBar` hide the title label and the buttons if the sub window is maximized. Always show the title and close button if not maximized. This is needed to reset the state correctly after maximization.

Add the helper method `SubWindow::addTitleButton` to reduce code repetition in the constructor.

Disable the minimize button by taking the current flags and removing the minimize button hint from them instead of giving a list which might become incomplete in the future. So only do what we want to do.

Remove a dependency on the `MdiArea` when checking if the sub window is the active one. Query its own window state to find out if it is active.

michaelgregorius · 2024-09-22T11:37:10Z

This PR is now ready for review as I have managed to fix the problems with the SubWindow class so that it now looks like in the screenshot above:

I have also added some other small improvements to the SubWindow class.

Fix a typo and add a newline to the end of the file.

sakertooth · 2024-09-24T00:26:34Z

@michaelgregorius, I have plans to revert a good chunk of #7453, which would affect this PR. I don't think we should keep moving forward with making SlicerT bigger/resizable for now until we properly rework the instrument window to support larger sizes the right way. Effect racks and (at the very least) Lv2 layouts do not render properly and the envelope tab is massively distorted.

We will always have another chance at making the instrument windows bigger, it's not like I am throwing that idea out completely. I just don't think its worth progressing further with this knowing the massive regressions we still have to deal with (and I do not like how resizability/bigger sizes for instruments was implemented).

michaelgregorius · 2024-09-24T17:39:47Z

@sakertooth, this pull request can be reviewed and merged nevertheless. The changes in src/gui/instrument/InstrumentTrackWindow.cpp prepare the instrument window to do "the right thing" once it is resizable and the changes in SubWindow fix a problem that exists in the class independent of instrument resizing.

So, I'd appreciate a review.

JohannesLorenz · 2024-09-29T12:36:55Z

This PR will conflict with #7524 . I would like to ask for time to consolidate the differences and commonalities to avoid merge conflicts and unncessary commits.

JohannesLorenz · 2024-10-01T20:28:59Z

As my PR from #7524 does not fully work after the SlicerT update and conflict with your PR, I will try to use your PR for Lv2 and then see what I need to change. Wish me luck 😄

@michaelgregorius As for your PR, isn't this two separate things in one PR? Do you think it would make sense to split this PR into "add maximize button" and "fix resizing behavior"?

michaelgregorius · 2024-10-03T13:07:26Z

As my PR from #7524 does not fully work after the SlicerT update and conflict with your PR, I will try to use your PR for Lv2 and then see what I need to change. Wish me luck 😄

Good luck then! 😅

@michaelgregorius As for your PR, isn't this two separate things in one PR? Do you think it would make sense to split this PR into "add maximize button" and "fix resizing behavior"?

Yes, it can definitively be separated as you have described. Therefore I have now separated the sub window fixes into pull request #7530. Do you want to review it?

JohannesLorenz · 2024-10-03T18:43:37Z

Do you want to review it?

Yes, thanks, I added myself to the reviewer list.

I'm already in the middle of testing, most things look good.

JohannesLorenz · 2024-10-08T23:40:55Z

Issue during testing:

New project
Create SlicerT (don't maximize)
Drag TripleOsc right into the SlicerT window

Problems:

TripleOsc has wrong size => Not fault of this PR
TripleOsc has maximize button => Should IMO be fixed in this PR

Variations:

Swap around TripleOsc and SlicerT
Ignore the "(don't maximize)"

=> In all cases, the maximize button should be updated to the new instrument
=> A maximized window should, after the instrument changes, only be maximized if the new instrument supports it

JohannesLorenz

What I did:

First functional review
Testing review

What needs to be done:

Style review
Second functional review (IMO optional)

src/gui/instrument/InstrumentTrackWindow.cpp

JohannesLorenz · 2024-10-08T23:33:29Z

src/gui/instrument/InstrumentTrackWindow.cpp

+
+	if (m_instrumentView->isResizable())
+	{
+		// TODO As of writing SlicerT is the only resizable instrument. Is this code specific to SlicerT?


I assume that the diff between InstrumentView (which contains SlicerT OR ZynAddSubFx OR TripleOsc etc) and the whole subwindow is just not dependent on the instrument (SlicerT), but it is rather:

The additional width is the small border left and right of SlicerT to form the whole SubWindow

The larger 208 height is for the PianoRoll below and the knobs etc above?

It might be interesting (but also terrifying) to check if the calculation matches the numbers from PianoView, InstrumentTrackWindow etc. 😆

Clear the `Qt::MSWindowsFixedSizeDialogHint` flag for resizable instruments (symmetric to the `else` case).

Update the title bar of an instrument's sub window if the model changes, e.g. if an instrument is exchanged via drag & drop. The main fix is to call the new method `updateSubWindowState` in `InstrumentTrackWindow::modelChanged`. It contains mostly the code that was previously executed in the constructor of `InstrumentTrackWindow`. The constructor now simply calls this method after it has put the constructed instance into a sub window. With the current implementation the sub window needs to be explicitly triggered to update its title bar once the flags have been adjusted in `updateSubWindowState`. This is done with the new public method `SubWindow::updateTitleBar`. Please note that such an explicit update is not needed if the instrument windows are managed by a `QMdiSubWindow` instead of a `SubWindow`. This means that the implementation of `SubWindow` is still missing something that `QMdiSubWindow` does. However, debugging also showed that setting the window flags of the sub window does not seem to lead to an event that could be caught in `SubWindow::changeEvent`. This was found out by simply dumping the event types of all events that arrive in that method and exchanging an instrument. The method `updateSubWindowState` uses the added method `findSubWindowInParents` to find the sub window it is contained in. The latter method should be considered to be moved into a templated helper class because it might be useful in other contexts as well. ## Technical details If you want to experiment with using QMdiSubWindows then simply add the following method to `MainWindow` (right next to `addWindowedWidget`): ``` QMdiSubWindow* MainWindow::addQMdiSubWindow(QWidget *w, Qt::WindowFlags windowFlags) { // wrap the widget in our own *custom* window that patches some errors in QMdiSubWindow auto win = new QMdiSubWindow(m_workspace->viewport(), windowFlags); win->setAttribute(Qt::WA_DeleteOnClose); win->setWidget(w); m_workspace->addSubWindow(win); return win; } ``` Then call that method instead of `addWindowedWidget` in the constructor of `InstrumentTrackWindow`: ``` QMdiSubWindow* subWin = getGUI()->mainWindow()->addQMdiSubWindow( this ); ``` You can then comment out the cast and the call of `updateTitleBar` in `updateSubWindowState` and everything will still work.

Show or hide the "Size" and "Maximize" entries in the system menu depending on whether the instrument view is resizable or not.

Show the sub windows of non-resizable instruments as normal if the sub window is maximized because it was previously used with a resizable instrument.

michaelgregorius · 2024-10-09T16:40:55Z

Issue during testing:
* New project

* Create SlicerT (don't maximize)

* Drag TripleOsc right into the SlicerT window
Problems:
* TripleOsc has wrong size => Not fault of this PR

* TripleOsc has maximize button => Should IMO be fixed in this PR
Variations:
* Swap around TripleOsc and SlicerT

* Ignore the "(don't maximize)"
=> In all cases, the maximize button should be updated to the new instrument => A maximized window should, after the instrument changes, only be maximized if the new instrument supports it

@JohannesLorenz, all issues except for the "TripleOsc has wrong size => Not fault of this PR" should be fixed by the last three commits.

…onForResizableInstruments Conflicts: * include/SubWindow.h * src/gui/SubWindow.cpp

JohannesLorenz

What I did:

First functional review

What needs to be done:

Testing review (I volunteer for it once the code is final)
Style review

JohannesLorenz · 2024-10-10T22:06:25Z

src/gui/instrument/InstrumentTrackWindow.cpp

+	while (p != nullptr)
+	{
+		auto mdiSubWindow = dynamic_cast<QMdiSubWindow*>(p);
+		if (mdiSubWindow)


These 2 are other good candidates for "inlining".

JohannesLorenz · 2024-10-10T22:08:43Z

src/gui/instrument/InstrumentTrackWindow.cpp

+
+		if (instrumentViewResizable)
+		{
+			// TODO As of writing SlicerT is the only resizable instrument. Is this code specific to SlicerT?


Note, this is still not fixed - see my previous comment.

src/gui/instrument/InstrumentTrackWindow.cpp

JohannesLorenz · 2024-10-10T23:31:32Z

src/gui/instrument/InstrumentTrackWindow.cpp

+	return nullptr;
+}
+
+void InstrumentTrackWindow::updateSubWindowState()


What I find strange about this method: It is no way specific to Instrument Track SubWindows (except the "isResizable" which I plan to generalize to both Instruments and Effects, i.e. to PluginView, but not all SubWindow types - nonetheless, SubWindow might just do a qobject_cast on its children, and if it's a PluginView, use the isResizable from there).

I guess your PR is only required for instruments, because they are the only SubWindow type that can be maximized and can both be resizable or not? In this case, the issue will occur for Effects sooner or later. And for the other SubWindow types, I assume it might not harm?

The advantage of putting that code into SubWindow would be cleaner encapsulation and no need to find the parent.

src/gui/instrument/InstrumentTrackWindow.cpp

Rename `updateSubWindowState` to `updateSubWindow`.

michaelgregorius · 2024-10-11T08:08:26Z

What I find strange about this method: It is no way specific to Instrument Track SubWindows (except the "isResizable" which I plan to generalize to both Instruments and Effects, i.e. to PluginView, but not all SubWindow types - nonetheless, SubWindow might just do a qobject_cast on its children, and if it's a PluginView, use the isResizable from there).

I guess your PR is only required for instruments, because they are the only SubWindow type that can be maximized and can both be resizable or not? In this case, the issue will occur for Effects sooner or later. And for the other SubWindow types, I assume it might not harm?

The advantage of putting that code into SubWindow would be cleaner encapsulation and no need to find the parent.

Moving this discussion out of the code thread, @JohannesLorenz.

Yes, it should definitively be generalized.

However, with the current hierarchy this would not really work well (I think you already mentioned some of the problems somewhere else). With regards to the instruments the PluginView only describes the instrument specific widget, e.g. the orange 3OSC window, via InstrumentView which inherits from PluginView. However, the child of the SubWindow is the InstrumentTrackWindow which described the full window with all tabs, etc. and which has rather unspecific inheritances (QWidget, ModelView and SerializingObjectHook).

So it seems that some more specific class would have to be inserted that InstrumentTrackWindow and whatever class deals with the effects can inherit from. Let's call it PluginMainView for now. It might even be that the isResizable method would then need to be moved into PluginMainView so that the sub window can query it (assuming that PluginMainView is the direct child of the sub window).

Regarding moving logic into the sub window. In that case I'd prefer a sub window class like PluginSubWindow which would inherit from SubWindow and receive a PluginMainView into its constructor and which would then operate on this instance. That way SubWindow would only deal with the technical aspects, e.g. how to render itself, whereas PluginSubWindow would be used for functional or "business" aspects.

I am still not sure why SubWindow exists though? What problems is it expected to solve? I am asking because due to its imperfect implementation it is also causing lots of problems. See for example the public method updateTitleBar which had to be introduced because it does not update automatically whereas a QMdiSubWindow does.

JohannesLorenz · 2024-10-11T17:59:55Z

I am still not sure why SubWindow exists though? What problems is it expected to solve?

Only to that specific question: Did you see the comment in the header (above the class) and especially in the cpp above SubWindow::moveEvent?

See commit 7cc917c . The author @Wallacoloo might want to say something about it, too.

Wallacoloo · 2024-10-11T20:18:21Z

That's 9 years ago, patching over a bug that existed in QMdiSubWindow at the time. If you're able to remove SubWindow and everything still appears to work, then it's probably been fixed upstream and is fine to remove. As far as the behavior to check for: does any combination of moving, resizing, and maximizing/minimizing a QMdiSubWindow (or the main LMMS window) cause the QMdiSubWindow to take on unexpected dimensions? For example, maximize the subwindow, then resize the main LMMS window (just a pixel or two), then un-maximize the subwindow: does it restore to its original size? There was some interaction (probably not exactly that, but like that) where it would lose track of its non-maximized dimensions and not restore to the size/location you'd expect it to.

…

On Fri, Oct 11 2024 at 11:03:19 AM -07:00:00, Johannes Lorenz ***@***.***> wrote: > I am still not sure why SubWindow exists though? What problems is it > expected to solve? > Only to that specific question: Did you see the comment in the header (above the class) and especially in the cpp above SubWindow::moveEvent? See commit 7cc917c <7cc917c> . The author @Wallacoloo <https://github.com/Wallacoloo> might want to say something about it, too. — Reply to this email directly, view it on GitHub <#7514 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJHS7YLZ4UO7BG7HNUHK7TZ3AHGPAVCNFSM6AAAAABOSZJRZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXHA4TANJYGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Wallacoloo · 2024-10-11T20:23:36Z

Oh, well look at the bug linked at the top of that file (thank you whoever linked it): <https://bugreports.qt.io/browse/QTBUG-256> It's closed, as "out of scope". No activity or Qt commits linking to it. It's probably still something you have to deal with on Qt+X11 (doesn't mention anything about Wayland).

…

On Fri, Oct 11 2024 at 08:18:06 PM +00:00:00, ***@***.*** wrote: That's 9 years ago, patching over a bug that existed in QMdiSubWindow at the time. If you're able to remove SubWindow and everything still appears to work, then it's probably been fixed upstream and is fine to remove. As far as the behavior to check for: does any combination of moving, resizing, and maximizing/minimizing a QMdiSubWindow (or the main LMMS window) cause the QMdiSubWindow to take on unexpected dimensions? For example, maximize the subwindow, then resize the main LMMS window (just a pixel or two), then un-maximize the subwindow: does it restore to its original size? There was some interaction (probably not exactly that, but like that) where it would lose track of its non-maximized dimensions and not restore to the size/location you'd expect it to. On Fri, Oct 11 2024 at 11:03:19 AM -07:00:00, Johannes Lorenz ***@***.***> wrote: >> I am still not sure why SubWindow exists though? What problems is >> it expected to solve? >> > Only to that specific question: Did you see the comment in the > header (above the class) and especially in the cpp above > SubWindow::moveEvent? > > See commit 7cc917c > <7cc917c> > . The author @Wallacoloo <https://github.com/Wallacoloo> might want > to say something about it, too. > > — > Reply to this email directly, view it on GitHub > <#7514 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAJHS7YLZ4UO7BG7HNUHK7TZ3AHGPAVCNFSM6AAAAABOSZJRZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXHA4TANJYGY>. > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

michaelgregorius · 2024-10-12T19:44:38Z

Thanks for the heads up, @Wallacoloo! I could not reproduce any of the problems while changing the state of the sub windows and the main window (under X11). However, I guess switching to QMdiSubWindow now would likely open another can of worms anyway as it does not seem to support style sheets very well.

JohannesLorenz · 2024-10-12T21:15:53Z

Regarding the rest of the Subwindow generalization discussion: This is all a bit complex, and might change drastically as soon as I change the hierarchy of PluginView/EffectControlDialog/EffectView. So I suggest either waiting until I have a PR here, or finish this PR as it is (open comment to solve, outstand test review). Ultimately, the decision is up to the author - I would accept both.

michaelgregorius · 2024-10-13T07:31:46Z

Regarding the rest of the Subwindow generalization discussion: This is all a bit complex, and might change drastically as soon as I change the hierarchy of PluginView/EffectControlDialog/EffectView. So I suggest either waiting until I have a PR here, or finish this PR as it is (open comment to solve, outstand test review). Ultimately, the decision is up to the author - I would accept both.

I'd say lets first do the hierarchy changes, hoping that they might make it easier or clearer how to solve the remaining problems with this PR.

JohannesLorenz · 2024-10-13T21:48:51Z

lets first do the hierarchy changes

I uploaded them into a PR now: #7544 .

michaelgregorius linked an issue Sep 20, 2024 that may be closed by this pull request

Maximize button in SlicerT #7512

Open

1 task

michaelgregorius mentioned this pull request Sep 20, 2024

Maximize button in SlicerT #7512

Open

1 task

michaelgregorius added 4 commits September 22, 2024 13:26

Add SubWindow::addTitleButton

f5d2544

Add the helper method `SubWindow::addTitleButton` to reduce code repetition in the constructor.

Only disable the minimize button

cdef4b5

Disable the minimize button by taking the current flags and removing the minimize button hint from them instead of giving a list which might become incomplete in the future. So only do what we want to do.

Remove dependency on MdiArea

0921c97

Remove a dependency on the `MdiArea` when checking if the sub window is the active one. Query its own window state to find out if it is active.

Typo and newline at end of file

4b0beb4

Fix a typo and add a newline to the end of the file.

michaelgregorius requested a review from sakertooth September 23, 2024 15:14

michaelgregorius mentioned this pull request Oct 3, 2024

Fix the maximization of sub windows #7530

Merged

JohannesLorenz reviewed Oct 8, 2024

View reviewed changes

michaelgregorius added 4 commits October 9, 2024 13:57

Clear Qt::MSWindowsFixedSizeDialogHint

e85699f

Clear the `Qt::MSWindowsFixedSizeDialogHint` flag for resizable instruments (symmetric to the `else` case).

Update the system menu

e69204f

Show or hide the "Size" and "Maximize" entries in the system menu depending on whether the instrument view is resizable or not.

Show non-resizable instruments as normal

9aab76b

Show the sub windows of non-resizable instruments as normal if the sub window is maximized because it was previously used with a resizable instrument.

Merge remote-tracking branch 'upstream/master' into 7512-MaximizeButt…

f677b8c

…onForResizableInstruments Conflicts: * include/SubWindow.h * src/gui/SubWindow.cpp

JohannesLorenz reviewed Oct 10, 2024

View reviewed changes

Fix typo

8b4976e

Rename updateSubWindowState

a594002

Rename `updateSubWindowState` to `updateSubWindow`.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maximize button for resizable instruments #7514

Maximize button for resizable instruments #7514

michaelgregorius commented Sep 20, 2024 •

edited

Loading

michaelgregorius commented Sep 21, 2024

michaelgregorius commented Sep 21, 2024

michaelgregorius commented Sep 22, 2024

michaelgregorius commented Sep 22, 2024

sakertooth commented Sep 24, 2024

michaelgregorius commented Sep 24, 2024

JohannesLorenz commented Sep 29, 2024

JohannesLorenz commented Oct 1, 2024 •

edited

Loading

michaelgregorius commented Oct 3, 2024

JohannesLorenz commented Oct 3, 2024

JohannesLorenz commented Oct 8, 2024

JohannesLorenz left a comment

JohannesLorenz Oct 8, 2024

michaelgregorius commented Oct 9, 2024

JohannesLorenz left a comment

JohannesLorenz Oct 10, 2024

JohannesLorenz Oct 10, 2024

JohannesLorenz Oct 10, 2024

michaelgregorius commented Oct 11, 2024

JohannesLorenz commented Oct 11, 2024 •

edited

Loading

Wallacoloo commented Oct 11, 2024 via email

Wallacoloo commented Oct 11, 2024 via email

michaelgregorius commented Oct 12, 2024

JohannesLorenz commented Oct 12, 2024 •

edited

Loading

michaelgregorius commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

Maximize button for resizable instruments #7514

Are you sure you want to change the base?

Maximize button for resizable instruments #7514

Conversation

michaelgregorius commented Sep 20, 2024 • edited Loading

michaelgregorius commented Sep 21, 2024

michaelgregorius commented Sep 21, 2024

michaelgregorius commented Sep 22, 2024

michaelgregorius commented Sep 22, 2024

sakertooth commented Sep 24, 2024

michaelgregorius commented Sep 24, 2024

JohannesLorenz commented Sep 29, 2024

JohannesLorenz commented Oct 1, 2024 • edited Loading

michaelgregorius commented Oct 3, 2024

JohannesLorenz commented Oct 3, 2024

JohannesLorenz commented Oct 8, 2024

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz Oct 8, 2024

Choose a reason for hiding this comment

michaelgregorius commented Oct 9, 2024

JohannesLorenz left a comment

Choose a reason for hiding this comment

JohannesLorenz Oct 10, 2024

Choose a reason for hiding this comment

JohannesLorenz Oct 10, 2024

Choose a reason for hiding this comment

JohannesLorenz Oct 10, 2024

Choose a reason for hiding this comment

michaelgregorius commented Oct 11, 2024

JohannesLorenz commented Oct 11, 2024 • edited Loading

Wallacoloo commented Oct 11, 2024 via email

Wallacoloo commented Oct 11, 2024 via email

michaelgregorius commented Oct 12, 2024

JohannesLorenz commented Oct 12, 2024 • edited Loading

michaelgregorius commented Oct 13, 2024

JohannesLorenz commented Oct 13, 2024

michaelgregorius commented Sep 20, 2024 •

edited

Loading

JohannesLorenz commented Oct 1, 2024 •

edited

Loading

JohannesLorenz commented Oct 11, 2024 •

edited

Loading

JohannesLorenz commented Oct 12, 2024 •

edited

Loading