Saving a project forces mixer/automation/* windows to background

[Wallacoloo]

This only occurs when you have one of the windows in LMMS' maximized, as shown in the gif below (most commonly, people do this with the song editor, but you can do it with the piano roll and automation editors too).

In this case, if I have any other window, like the Mixer, in the foreground and I hit Ctrl+S (or save manually via the file menu), such windows will be sent to the background. It can be an annoyance.

It is interesting to note that this still occurs if the automation editor or piano roll is maximized instead of the song editor, but there is not issue if no window is maximized.

[Wallacoloo]

What's more, this seems to also happen whenever LMMS autosaves - I left the window idle with the mixer on top and every minute on the dot the mixer would be sent to the background (and I would recall it to the foreground manually). Again, this only happened when I had another window inside LMMS maximized.

FURTHERMORE, if another application has focus during the autosave, LMMS forces itself to the foreground (I'm using XFCE as my desktop environment), but again, only if one of the LMMS sub-windows is maximized. This has been plaguing me for weeks, so I'm glad I now know a workaround and have narrowed down the cause.

[softrabbit]

I think the first spot where this happens is here:
https://github.com/LMMS/lmms/blob/master/src/core/Song.cpp#L1076

Something in those saveState() calls does it. Tried commenting out the call to gui->pianoRoll()->saveState() and LMMS stopped behaving badly when the piano roll was maximised. Will drill deeper later, unless someone else beats me to it.

So, we end up here: https://github.com/LMMS/lmms/blob/master/src/gui/MainWindow.cpp#L696, which I guess is a workaround for QWidget::normalGeometry() not working as advertised. (at least not on 4.8.6 that I just tried) 😦

[musikBear]

fyi: not reproduceable on 1.1.3 win 32
(I strongly favor a better behavior for 'smaller windows', in order to bring them forward, and have them staying there. Imo that could be default behavior, but a small 'tag-icon' in the window tittle-bar, could work. It is to cumbersome to use the context-menu for every window that opens (imo:) )

[softrabbit]

Maybe every window should store its geometry every time it gets a resize or move event and isn't maximized or minimized. And then save that without needing the maximization trickery.

[Wallacoloo]

Ohhh. Interesting. So the purpose of the lines around 696 in MainWindow.cpp is to also save the non-maximized/minimized geometry so that pressing the window's restore button will work the same now as when you load the file at a later time.

Perhaps there's a variant of showMaximized() that doesn't call the maximized window to focus? Maybe just setWindowState?

Edit: the documentation for setWindowState says that it will cause the window to be hidden for some reason. Thus a followup call to show() is required, which I think wouldn't fix anything.

Edit 2: setWindowState does not cause the window to be hidden on my system.

[Wallacoloo]

The other option is to enumerate the saveWidgetState() calls by widget depth and hide()/show() every window so that the order is still retained afterward. You may have to additionally direct focus back to the window that originally had focus (not sure). It's a bit wasteful and hacky though, so I think saving the geometry upon resize events would be preferable to this option.

At the very least, we can call _w->setAttribute(Qt::WA_ShowWithoutActivating) before the current call to showNormal/Maximized/Minimized so as not to steal focus from other applications, as according to this post on StackOverflow.

Edit: _w->setAttribute(Qt::WA_ShowWithoutActivating) does not appear to fix focus stealing nor window ordering.

[Wallacoloo]

By the way, the normalGeometry() issue seems to be a relatively obscure bug, only affecting X11 users when the window is maximized by the user and not by a Qt call (although my tests indicate that it the issue still occurs when the window is maximized by Qt code) (bug report). A user in that bug report suggests doing what amounts to your suggestion of storing the size manually.

I did some digging to try to figure out how showNormal() figures out how to position the window even though normalGeometry() reports QRect(0,0, -1x-1).

Turns out that if you replace all the show()/hide() stuff in MainWindow::saveWidgetState() with calls like:

if( mined || maxed ) {
_w->setWindowState(_w->windowState() & ~(Qt::WindowMinimized
                                     | Qt::WindowMaximized
                                     | Qt::WindowActive));
}
...
if (maxed)
{
    _w->setWindowState((_w->windowState() | Qt::WindowMaximized) & ~Qt::WindowActive);
}
if (mined)
{
    _w->setWindowState((_w->windowState() | Qt::WindowMinimized) & ~Qt::WindowActive);
}

... then you can achieve the exact same behavior as currently, including the improper window reordering - even if you comment out the last two if statements. I tracked down Qt's setWindowState function for X11 here. For the case of a restore operation (as in the above), it's pretty simple. If basically amounts to this:

<Save the old state>
<Store the new state as a member variable.>
qt_change_net_wm_state(this, (newstate & Qt::WindowMaximized),
                                       ATOM(_NET_WM_STATE_MAXIMIZED_HORZ),
                                       ATOM(_NET_WM_STATE_MAXIMIZED_VERT));  
if (needShow) // needShow should be computed as false based on me mentally following the code flow
        show();

if (newstate & Qt::WindowActive) // WindowActive was set to false in the attribute list
        activateWindow();

QWindowStateChangeEvent e(oldstate);
QApplication::sendEvent(this, &e);

There are of course some more function calls used to determine control flow, but none of them should cause any state changes. It basically narrows the issue down to either the qt_change_net_wm_state() or QApplication::sendEvent() calls. Manually broadcasting the event and not calling this function does not replicate issue, but this may be because the Widget's internal state isn't actually changed, which it might expose to the event receivers elsewhere. But there is a real possibility that the problems are raised by the actual call into X11 (qt_change_net_wm_state is a thin wrapper for an X11 call).

It is worth noting that although changing the code to setWindowState() calls still suffers from the window reordering problem, it no longer causes LMMS to steal focus from other applications unless you remove the code that nulls the Qt::WindowActive fields (which I think may be necessary to ensure the correct window retains focus, unfortunately).

So yeah, I think the only proper fix is to track the non-maximized geometry ourselves, as @softrabbit suggested. Sadly, I think this requires subclassing a bunch of Widgets from a new custom class that tracks this information. The only other feasible option is a global event filter with a map that retains this information, or creating some slots when these widgets are created for whatever slot gets fired when a widget's geometry is changed, but I think the simpler option should be favored here even if it requires editing more code.

[Wallacoloo]

I did a bit of toying around with a solution today. I'm not intimately familiar with Qt, but it turns out that all of the LMMS subwindows are created by calls to gui->mainWindow()->workspace()->addSubWindow(x). This corresponds to QMidArea::addSubWindow, which accepts either a QWidget or a QMdiSubWindow object. In the former case, it will simply instantiate a QMdiSubWindow for the widget. All of our calls currently fall into the former case (passing a QWidget).

So the easiest/best way to fix this is likely to add a new class, SubWindow or similar, that inherits from QMdiSubWindow and handles all resize/move events to keep track of the normal geometry. And then abstract the gui->mainWindow()->workspace()->addSubWindow(x) call to just gui->mainWindow()->addSubWindow(QWidget *), and wrap the widget in our custom subwindow before forwarding it to the MdiArea addSubWindow function.

Lastly, modify the logic inside of MainWindow::saveWidgetState. It currently has a block that looks like this:

if( _w->widget()->parentWidget() != NULL &&
    _w->widget()->parentWidget()->inherits( "QMdiSubWindow" ) )
{
    _w = _w->parentWidget();
}

It should only take some minor modifications to make it also use some custom accessors when we inherit from "SubWindow" in place of Qt's x(), y(), etc.

I should hopefully have something ready within a day or two.

[Wallacoloo]

Proposed fix via PR #2103.

[Wallacoloo]

Closed via #2103. Hurray!