-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix the maximization of sub windows #7530
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.
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.
This reverts commit 9a1eaac.
Make sure that `Qt::CustomizeWindowHint` is set like it was before.
@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. |
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. |
There was a problem hiding this 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)
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.
@Rossmaxx, can you do a style review as proposed by @JohannesLorenz here? |
There was a problem hiding this 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.
Fix whitespaces in parameter list of `addTitleButton`.
@JohannesLorenz, @Rossmaxx, I have merged this PR so that I can in turn merge |
Very good, that makes #7514 much more readable! |
## Fix rendering of maximized sub windows ### Adjustments in paintEvent In `SubWindow::paintEvent` don't paint anything if the sub window is maximized . Otherwise some gradients are visible behind the maximized child content. ### Adjustments in adjustTitleBar 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. 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. ### Add method addTitleButton Add the helper method `SubWindow::addTitleButton` to reduce code repetition in the constructor. ### Other changes 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. When calling `setWindowFlags` in the constructor only adjust the existing flags with the changes that we actually want to do. It was ensured that all other flags that have been set before still apply with this change.
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.