fix the maximize behavior of subwindows on Mac #2798

BaraMGB · 2016-05-25T21:05:34Z

#2450 (comment) @tresf reported an issue on mac. The subwindow don't maximize proper. The button don't change from maximize to restore. This PR fix this.

edit Explanation:

I added a function for testing if the subwindow is maximized. It's quite simple: it checks if the subwindow size is identically to the size of the MdiArea. But we have to care about the Scrollbars of the MdiArea. If an other Subwindow is in Front of the maximized Subwindow, the MdiArea has Scrollbars (sometimes horizontal, sometimes vertical). Our Subwindow becomes smaller than the MdiArea although it is maximized.

BaraMGB · 2016-05-25T21:15:54Z

@tresf please test out what happens if more than one window are maximized.

maximize the songeditor.
click on bb-editor button in the maintoolbar to bring it in front.
maximize the bb-editor, too.

I hope, everything goes right with this PR.

tresf · 2016-05-25T22:45:25Z

The behavior is quite bazaar. It seems that Qt on Mac honors the "Only one window can be maximized at a time" behavior. Which can leave the button in a strange situation where it does nothing... 9 out of 10 times the behavior seems correct though although I did get it to lock in the "restore" mode where the maximize pixmap was displayed indefinitely.

It seems when the maximized window gains focus, it usually goes back to the proper "maximize" mode with the restore pixmap displayed.

I still question if this is the right way. It seems we could bind the maximize and restore buttons to whatever the context menu action is, but I haven't delved into the code that much.

BaraMGB · 2016-05-25T22:57:37Z

No, I think there is something with the scroll bar on your first screeny. I guess, the window size is smaller than the MdiArea size and the test fails. 😣

BaraMGB · 2016-05-27T09:01:32Z

@tresf : I have updated this PR. And I edit a explanation in the description above. Please check out the branch. Thank you for testing this.

liushuyu · 2016-05-27T14:09:40Z

It seems we could bind the maximize and restore buttons to whatever the context menu action is, but I haven't delved into the code that much.

I still wonder if the context menu is handled by window manager, since the MDI subwindow is inherited from MainWindow.

BaraMGB · 2016-05-27T14:43:26Z

I'm sorry, if I've messed someone up, but this has nothing to do with context menu. It only implements a new function which tests if the subwindow is maximized (see above, first post)

regards

tresf · 2016-05-27T15:24:45Z

dee95b0 👍

tresf · 2016-05-27T15:27:45Z

src/gui/SubWindow.cpp

+#ifdef LMMS_BUILD_APPLE
+		if( isMaximizedOnMac() )
+		{
+			m_maximizeBtn->hide();


Is the ->hide() redundant? It seems you do it on every resize regardless, no?

BaraMGB · 2016-05-27T21:40:28Z

Is the ->hide() redundant? It seems you do it on every resize regardless, no?

yes. 👍 , and I found another one. Update

tresf · 2016-05-27T22:07:35Z

src/gui/SubWindow.cpp

@@ -121,6 +123,23 @@ void SubWindow::elideText( QLabel *label, QString text )



+bool SubWindow::isMaximizedOnMac()
+{
+	// we check if the subwindow size is identically to the MdiArea size


This can be simplified.

// check if subwindow size is identical to the MdiArea size, accounting for scrollbars

BaraMGB · 2016-05-28T10:50:57Z

update

tresf · 2016-05-28T14:42:35Z

src/gui/SubWindow.cpp

@@ -220,22 +235,34 @@ void SubWindow::resizeEvent( QResizeEvent * event )
 	{
 		buttonBarWidth = buttonBarWidth + m_buttonSize.width() + buttonGap;
 		m_maximizeBtn->move( middleButtonPos );
+	// on Mac the isMaximize() function returns always false


Mentioning an unused function can confuse people when they read this later.

Should we be using isMaximize() instead of WindowMaximizeButtonHint?

If so, should we override this on Mac only and leave the logic identical between function calls?

The reason I ask is that the logic is for Mac-only yet we allow some portions to execute every resize event (such as && !isMaximizedOnMac(). Is that desired?

I'm unsure if I understand you right here. Qt::WindowMaximizeButtonHint is a Flag which returns true if the window is able to maximize. For example the Fx-Mixer window is not able to be maximized therefore this flag returns false. And if it's false we don't need the maximize button.

We could implement the isMaximized() function in this form:

bool SubWindow::isMaximized() { #ifdef LMMS_BUILD_APPLE // check if subwindow size is identical to the MdiArea size, accounting for scrollbars int hScrollBarHeight = mdiArea()->horizontalScrollBar()->isVisible() ? mdiArea()->horizontalScrollBar()->size().height() : 0; int vScrollBarWidth = mdiArea()->verticalScrollBar()->isVisible() ? mdiArea()->verticalScrollBar()->size().width() : 0; QSize areaSize( this->mdiArea()->size().width() - vScrollBarWidth, this->mdiArea()->size().height() - hScrollBarHeight ); return areaSize == this->size(); #else return QMdiSubWindow::isMaximized(); #endif }

So we have one function for all systems. But for what I see at the first look, we have on Linux & Co. one more redundant function call. I'll look tomorrow into it.

Regards

tresf · 2016-05-28T14:43:45Z

Testing still passes. I left some more comments about the code approach. 👍

BaraMGB · 2016-06-03T13:24:28Z

Okay, we have now one isMaximized() function for all systems. @tresf

Umcaruje · 2016-06-03T14:27:24Z

@BaraMGB the code looks good 👍

tresf · 2016-06-04T05:48:14Z

src/gui/SubWindow.cpp

 		{
 			m_minimizeBtn->move( middleButtonPos );
 		}
 		else
 		{
 			m_minimizeBtn->move( leftButtonPos );
 		}
-		m_minimizeBtn->show();
-		m_restoreBtn->hide();
 		if( isMinimized() )
 		{
 			if( m_maximizeBtn->isHidden() )


This entire if/else can be simplified....

m_restoreBtn->move( m_maximizeBtn->isHidden() ? middleButtonPos : leftButtonPos );

tresf · 2016-06-04T05:50:56Z

No objections, but I still have yet to test it out on OS X.

BaraMGB · 2016-06-04T07:20:09Z

update.

tresf · 2016-06-04T15:10:42Z

src/gui/SubWindow.cpp

+		m_restoreBtn->move( middleButtonPos );
+		if( isMaximized() )
+		{
+			m_restoreBtn->show();


Perhaps...

m_restoreBtn->setVisible( isMaximized() ); m_maximizeBtn->setVisible( !isMaximized() );

May also remove the need to hide everything at the top of the function.

Just a side note... I'm not sure how well the optimizers handle this type of logic, but down the road isMaximized() may benefit from being cached into a variable for the resize event if we ever are in a scenario where we do a bunch of window resizing (such as on project load, etc). Minor point though.... otherwise, the wasted CPU cycles should be relatively benign.

tresf · 2016-06-04T15:18:19Z

Tested, works fine. Minor note, the restore button can have some strange behavior if restoring two maximized windows one-after the other. The hiding of the scrollbar seems to nudge the min/max/restore buttons to the right when it gains focus and it ignores the click until the scrollbar hides, then it works fine. I'm not sure if this is easy to fix and I don't want to beat it up, so this is ok to merge.

I did make a comment on the use of setVisible versus show/hide. I think in some cases, you can simplify the logic even more by calling setVisible (or alternately setHidden) directly rather than using an if/else block.

liushuyu · 2016-06-04T16:36:52Z

Just a side note... I'm not sure how well the optimizers handle this type of logic

AFAIK, Modern compilers can optimize these things quite well, especially when combined with PGO (Profile Guided Optimization) (Note: No luck with LMMS, the optimization changes will crash LMMS).

BaraMGB · 2016-06-04T21:08:21Z

The hiding of the scrollbar seems to nudge the min/max/restore buttons to the right when it gains focus and it ignores the click until the scrollbar hides, then it works fine.

Can you provide a screenshot?

BaraMGB · 2016-06-05T11:09:53Z

I did make a comment on the use of setVisible versus show/hide.

isMaximized() may benefit from being cached into a variable for the resize event

👍

Add some more code optimisations.

tresf · 2016-06-07T04:39:19Z

👍 Code optimizations look great however something fishing is happening with the restore button now...

BaraMGB · 2016-06-07T13:03:45Z

I try to install a build enviroment for this in a VM. I hope it works like expected and I can install OS10.10 for this.

Regards

tresf · 2016-06-07T14:12:03Z

You do not need to do this. I can continue testing for you.

BaraMGB · 2016-06-22T12:23:54Z

@tresf Okay, now the restore button should appears again.

tresf · 2016-06-23T02:49:59Z

👍

PhysSong · 2019-02-10T05:44:16Z

Just for references, it turned out that #488 (comment) is the reason for the wrong behavior. #4819 should be the correct way.

fix the maximize behavior of subwindows on Mac

BaraMGB force-pushed the fixMacSubwindow branch from 8a787a3 to dee95b0 Compare May 27, 2016 08:46

tresf reviewed May 27, 2016
View reviewed changes

BaraMGB force-pushed the fixMacSubwindow branch from dee95b0 to 16e750e Compare May 27, 2016 21:38

tresf reviewed May 27, 2016
View reviewed changes

BaraMGB force-pushed the fixMacSubwindow branch from 16e750e to 8b7a850 Compare May 28, 2016 10:49

tresf reviewed May 28, 2016
View reviewed changes

BaraMGB force-pushed the fixMacSubwindow branch from 8b7a850 to 62a4020 Compare June 3, 2016 13:22

tresf reviewed Jun 4, 2016
View reviewed changes

BaraMGB force-pushed the fixMacSubwindow branch from 62a4020 to 9f7990a Compare June 4, 2016 07:19

tresf reviewed Jun 4, 2016
View reviewed changes

BaraMGB force-pushed the fixMacSubwindow branch from 9f7990a to 8570c7d Compare June 5, 2016 11:05

fix the maximize behavior of subwindows on Mac

ee8ff0a

BaraMGB force-pushed the fixMacSubwindow branch from 8570c7d to ee8ff0a Compare June 22, 2016 12:22

tresf merged commit 36bb163 into LMMS:master Jun 23, 2016

PhysSong mentioned this pull request Feb 10, 2019

Ensure the restore button of a subwindow shows up correctly #4819

Merged

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Merge pull request LMMS#2798 from BaraMGB/fixMacSubwindow

9f03fbb

fix the maximize behavior of subwindows on Mac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the maximize behavior of subwindows on Mac #2798

fix the maximize behavior of subwindows on Mac #2798

BaraMGB commented May 25, 2016 •

edited

Loading

BaraMGB commented May 25, 2016

tresf commented May 25, 2016 •

edited

Loading

BaraMGB commented May 25, 2016

BaraMGB commented May 27, 2016

liushuyu commented May 27, 2016

BaraMGB commented May 27, 2016

tresf commented May 27, 2016

tresf May 27, 2016

BaraMGB commented May 27, 2016

tresf May 27, 2016 •

edited

Loading

BaraMGB commented May 28, 2016

tresf May 28, 2016 •

edited

Loading

BaraMGB May 28, 2016

tresf commented May 28, 2016

BaraMGB commented Jun 3, 2016

Umcaruje commented Jun 3, 2016

tresf Jun 4, 2016

tresf commented Jun 4, 2016

BaraMGB commented Jun 4, 2016

tresf Jun 4, 2016

tresf commented Jun 4, 2016 •

edited

Loading

liushuyu commented Jun 4, 2016

BaraMGB commented Jun 4, 2016

BaraMGB commented Jun 5, 2016 •

edited

Loading

tresf commented Jun 7, 2016

BaraMGB commented Jun 7, 2016

tresf commented Jun 7, 2016

BaraMGB commented Jun 22, 2016

tresf commented Jun 23, 2016

PhysSong commented Feb 10, 2019

fix the maximize behavior of subwindows on Mac #2798

fix the maximize behavior of subwindows on Mac #2798

Conversation

BaraMGB commented May 25, 2016 • edited Loading

BaraMGB commented May 25, 2016

tresf commented May 25, 2016 • edited Loading

BaraMGB commented May 25, 2016

BaraMGB commented May 27, 2016

liushuyu commented May 27, 2016

BaraMGB commented May 27, 2016

tresf commented May 27, 2016

tresf May 27, 2016

Choose a reason for hiding this comment

BaraMGB commented May 27, 2016

tresf May 27, 2016 • edited Loading

Choose a reason for hiding this comment

BaraMGB commented May 28, 2016

tresf May 28, 2016 • edited Loading

Choose a reason for hiding this comment

BaraMGB May 28, 2016

Choose a reason for hiding this comment

tresf commented May 28, 2016

BaraMGB commented Jun 3, 2016

Umcaruje commented Jun 3, 2016

tresf Jun 4, 2016

Choose a reason for hiding this comment

tresf commented Jun 4, 2016

BaraMGB commented Jun 4, 2016

tresf Jun 4, 2016

Choose a reason for hiding this comment

tresf commented Jun 4, 2016 • edited Loading

liushuyu commented Jun 4, 2016

BaraMGB commented Jun 4, 2016

BaraMGB commented Jun 5, 2016 • edited Loading

tresf commented Jun 7, 2016

BaraMGB commented Jun 7, 2016

tresf commented Jun 7, 2016

BaraMGB commented Jun 22, 2016

tresf commented Jun 23, 2016

PhysSong commented Feb 10, 2019

BaraMGB commented May 25, 2016 •

edited

Loading

tresf commented May 25, 2016 •

edited

Loading

tresf May 27, 2016 •

edited

Loading

tresf May 28, 2016 •

edited

Loading

tresf commented Jun 4, 2016 •

edited

Loading

BaraMGB commented Jun 5, 2016 •

edited

Loading