Maximize button for resizable instruments

[michaelgregorius]

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:

[michaelgregorius]

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]

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]

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

[michaelgregorius]

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.

[sakertooth]

@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]

@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]

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]

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 :smile:

@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]

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! :sweat_smile:

@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 https://github.com/LMMS/lmms/pull/7530. Do you want to review it?

[JohannesLorenz]

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]

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

[michaelgregorius]

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.

[michaelgregorius]

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]

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 7cc917cc06e44b09378d4e4e166f2dbf148a1e27 . The author @Wallacoloo might want to say something about it, too.

[Wallacoloo]

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 https://github.com/LMMS/lmms/commit/7cc917cc06e44b09378d4e4e166f2dbf148a1e27 . The author @Wallacoloo https://github.com/Wallacoloo might want to say something about it, too.

— Reply to this email directly, view it on GitHub https://github.com/LMMS/lmms/pull/7514#issuecomment-2407890586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJHS7YLZ4UO7BG7HNUHK7TZ3AHGPAVCNFSM6AAAAABOSZJRZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXHA4TANJYGY. You are receiving this because you were mentioned.Message ID: @.***>

[Wallacoloo]

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 https://github.com/LMMS/lmms/commit/7cc917cc06e44b09378d4e4e166f2dbf148a1e27 . The author @Wallacoloo https://github.com/Wallacoloo might want to say something about it, too.

— Reply to this email directly, view it on GitHub https://github.com/LMMS/lmms/pull/7514#issuecomment-2407890586, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJHS7YLZ4UO7BG7HNUHK7TZ3AHGPAVCNFSM6AAAAABOSZJRZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBXHA4TANJYGY. You are receiving this because you were mentioned.Message ID: @.***>

[michaelgregorius]

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]

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]

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]

lets first do the hierarchy changes

I uploaded them into a PR now: #7544 .