Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the maximization of sub windows #7530

Merged

Conversation

michaelgregorius
Copy link
Contributor

Pull request #7514 more or less consists of two fixes. The first one fixes the rendering of maximized SubWindow instances and the second one enables instrument windows to be maximizable. This pull request separates out the first fix into its own pull request.

Technically its the same pull request as #7514 but commit 9a1eaac has been reverted in this branch.

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.
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.
Fix a typo and add a newline to the end of the file.
Make sure that `Qt::CustomizeWindowHint` is set like it was before.
@michaelgregorius
Copy link
Contributor Author

@JohannesLorenz, it seems to me that most of the problems that you have listed should not be caused by the changes in this PR as it is intended to only fix the rendering of maximized sub windows and does not introduce any capabilities for windows to maximize. Enabling to maximize instrument windows is done by the following pull request which needs to be merged with master once this PR is merged: #7514.

Do all the problems that you have listed disappear if you rebase your branch on the current master? If that's not the case then let's please not mix up pull requests, what they do and what should be fixed before they are merged. It seems that most of your remarks should be discussed in #7514 or similar PRs.

I have added commit 6b953fc which makes sure that all flags that have previously been set are also set in the new code, i.e. Qt::SubWindow, Qt::WindowMaximizeButtonHint, Qt::WindowSystemMenuHint, Qt::WindowCloseButtonHint, Qt::CustomizeWindowHint. The first four flags are already set as window flags when the code reaches the point to configure the flags, so only Qt::CustomizeWindowHint had to be added explicitly.

@JohannesLorenz
Copy link
Contributor

Do all the problems that you have listed disappear if you rebase your branch on the current master? If that's not the case then let's please not mix up pull requests

That's totally correct. Thanks for the hint! I will remove my comments soon and will probably just append them to issue #7510 . Will continue the review and testing.

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I did:

  • First functional review
  • Testing review (No issues found, but note there is one issue in the related PR - not 100% sure which PR it belongs to)

What needs to be done:

  • Style review
  • Second functional review (IMO optional)

src/gui/SubWindow.cpp Show resolved Hide resolved
src/gui/SubWindow.cpp Show resolved Hide resolved
Remove some calls to `isMaximized` where we already know that the sub
window is not maximized, i.e. where these calls would always return
`false`.

One adjustment would have resulted in a call to `setHidden(false)`. This
was changed to `setVisible(true)` to get rid of the double negation.

The other `false` would have gotten in an or statement and thus could be
removed completely.
@michaelgregorius
Copy link
Contributor Author

@Rossmaxx, can you do a style review as proposed by @JohannesLorenz here?

Copy link
Contributor

@Rossmaxx Rossmaxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New code mostly seems to follow style guidelines, these 2 are minor nitpicks, which you can avoid if you don't want to do.

include/SubWindow.h Outdated Show resolved Hide resolved
src/gui/SubWindow.cpp Outdated Show resolved Hide resolved
Fix whitespaces in parameter list of `addTitleButton`.
@michaelgregorius michaelgregorius merged commit 378ff8b into LMMS:master Oct 10, 2024
11 checks passed
@michaelgregorius michaelgregorius deleted the 7512-FixMaximizationOfSubWindow branch October 10, 2024 08:37
@michaelgregorius
Copy link
Contributor Author

@JohannesLorenz, @Rossmaxx, I have merged this PR so that I can in turn merge master back into #7514 (done with commit f677b8c).

@JohannesLorenz
Copy link
Contributor

Very good, that makes #7514 much more readable!

@michaelgregorius
Copy link
Contributor Author

Very good, that makes #7514 much more readable!

Yes, due to splitting it up into two PRs #7514 now is only about giving instrument windows the ability to maximize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants