Maximize button for resizable instruments #7514
Conversation
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.
|
Ok, this one is weird. I have added the following code snippet to 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? |
|
This seems to be caused by LMMS' own |
|
Maximizing also works as expected when the instrument window is added as a "normal"
|
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.
|
This PR is now ready for review as I have managed to fix the problems with the
I have also added some other small improvements to the |
Fix a typo and add a newline to the end of the file.
|
@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). |
|
@sakertooth, this pull request can be reviewed and merged nevertheless. The changes in So, I'd appreciate a review. |
|
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. |
|
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"? |
Good luck then! 😅
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? |
Yes, thanks, I added myself to the reviewer list. I'm already in the middle of testing, most things look good. |
|
Issue during testing:
Problems:
Variations:
=> In all cases, the maximize button should be updated to the new instrument |
|
|
||
| if (m_instrumentView->isResizable()) | ||
| { | ||
| // TODO As of writing SlicerT is the only resizable instrument. Is this code specific to SlicerT? |
There was a problem hiding this comment.
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.
@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
| while (p != nullptr) | ||
| { | ||
| auto mdiSubWindow = dynamic_cast<QMdiSubWindow*>(p); | ||
| if (mdiSubWindow) |
There was a problem hiding this comment.
These 2 are other good candidates for "inlining".
There was a problem hiding this comment.
As this is only a style issue which is a matter of taste I'd rather keep it as is. I find it more readable and due to the braces it is also not very prone to "Heartbleed-like" issues if code is added.
|
|
||
| if (instrumentViewResizable) | ||
| { | ||
| // TODO As of writing SlicerT is the only resizable instrument. Is this code specific to SlicerT? |
There was a problem hiding this comment.
Note, this is still not fixed - see my previous comment.
Rename `updateSubWindowState` to `updateSubWindow`.
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 So it seems that some more specific class would have to be inserted that Regarding moving logic into the sub window. In that case I'd prefer a sub window class like I am still not sure why |
Only to that specific question: Did you see the comment in the header (above the class) and especially in the cpp above See commit 7cc917c . The author @Wallacoloo might want to say something about it, too. |
|
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:
***@***.***>
|
|
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:
> ***@***.***>
>
|
|
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 |
|
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 |
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. |
I uploaded them into a PR now: #7544 . |
|
IMO the comment from @SpomJ can be ignored, as this would, if even, be handled in #7524 (see my comment to their comment). We are also a bit under time pressure (sorry, partially my fault), because this PR could influence #7201 .
@michaelgregorius We were stopping here with the review: #7514 (comment) . Since my hiearchy changes are not going to be realized we should continue there: There are still open comments from the previous review, and then the PR must be tested. Does that make sense? |
…onForResizableInstruments
|
@JohannesLorenz, I have merged the current master and did a quick test. Maximization is still working. Is there anything else to do? |
|
As noted before this PR fixes some general problems with maximizing Instrument windows correctly. IMO we can merge it and if the maximization feature is not wanted then it would suffice to only remove the code that does something with the |
michaelgregorius
deleted the
7512-MaximizeButtonForResizableInstruments
branch
|
@michaelgregorius Sorry, abusing this PR for contacting you. I am just going through the conditions to resizability of Btw, I remember you once wrote how I can contact you? Sorry, I do not find the info anymore. If you want to discuss, maybe an instant messenger might be better than reviving an old PR 😆 Though I don't have a strong opinion here, either. |
|
@JohannesLorenz, if this does not break anything else then it might also be worth a try. I guess this would mean that the method Every class that now overrides The code in Only thing I am not sure about is in how far the size policy of a widget is specific to layout contexts or if it is a general property. However, I guess it is also evaluated if a widget is for example a top-level widget that's not inside a layout. So it might be general anyway. I think I proposed to discuss things in Jitsi Meet because I assumed a "real" meeting. However, I cannot find that issue/comment anymore, either. 😅 Alternatively you can always ping me here in the relevant issues. |
|
@michaelgregorius I have just read a lof of Qt stuff. Finally, (I think) I found out: While
When I tried to set the sizePolicy of a
All in all, I guess I will rather use
I just found out that this seems to work great in |
|
@JohannesLorenz, Qt's use of sizes and sizing is definitively a complex topic so it's easy to get lost in there for hours. 😅 I think The size policy on the other hand is mainly used by other "clients" to inquire a widget about its capabilities with regards to making use of available space and its need for space. Example for clients are layout algorithms which need to check how to distribute available space to widgets. So it is mainly something that's interesting for "others". To me it always feels a bit like one thing is "driven" from the top whereas the other is driven from the bottom. |
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: