Bugs with toggling One Instrument Track Window Mode

[Hey-Holokin]

I, along with one other person, have found bugs when toggling this feature.

To reproduce my own issue regarding the feature, perform the following steps (may be dependent on other fundamental data which i was unable to gather at the time):
Open LMMS with the "One instrument track window mode" setting on (located in the first tab of the Settings window)
Open an instrument track window
Turn the aforementioned setting off
Attempt to open a different instrument track window
LMMS will then crash

@tresf has also observed bugs with this feature.

Operating system: Windows 10
LMMS Version: 1.2.0 rc3

[PhysSong]

Here's the backtrace:

(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00007ffff4b49425 in QObject::connect(QObject const*, char const*, QObject const*, char const*, Qt::ConnectionType) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x00000000005a884f in ModelView::doConnections() ()
#3  0x00000000005a8b86 in ModelView::setModel(Model*, bool) ()
#4  0x0000000000624afc in InstrumentTrackView::getInstrumentTrackWindow() ()
#5  0x0000000000624e30 in InstrumentTrackView::toggleInstrumentWindow(bool) ()
#6  0x00007ffff4b42d2a in QMetaObject::activate(QObject*, int, int, void**) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007ffff7a22312 in QAbstractButton::toggled(bool) ()
   from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5

[josh-audio]

I'm having trouble reproducing this bug on master. It doesn't appear that the single instrument window setting updates without a complete restart of LMMS. Could someone confirm this?

[PhysSong]

I've seen this issue, see the comment above.

[PhysSong]

Steps to reproduce:

1. Turn on the setting
2. Add four tracks(they shouldn't be opened)
3. Repeatedly open first two tracks(10 times will be enough)
4. Turn off the setting
5. Try to open the other tracks

I guess current window caching system is bug-prone with toggling the setting. I'll look into this once I have enough time.

[PhysSong]

Before fixing the bug, I'd like to ask opinions about desired behavior when the mode is turned on while multiple windows are being opened:

• Close every instrument window except last focused one?
• Ore Leave them as-is before user opens a new window?

[josh-audio]

I would prefer the first one, that way the user knows right away that their change had an effect. I can see the second option causing some confusion.

[PhysSong]

I succeeded to fix this bug. However, I'd like to ask opinion about keeping InstrumentTrackWindow caching. Currently I can't find major advantage of current caching logic, but I think it can be improved later. Should I

• Keep the caching system and fix the bug,
• Or remove entire caching system?

[JohannesLorenz]

With caching, you mean that opening an instrument, then closing, then opening another will no create a second ITW, but will make the first ITW visible again?

As this will probably be fixed on stable-1.2, I'm rather for a smaller solution with less risks.

[PhysSong]

With caching, you mean that opening an instrument, then closing, then opening another will no create a second ITW, but will make the first ITW visible again?

Yes, but it works in a very odd way. If the one instrument track mode is active, opening another instrument will create duplicated cache entries. It caused the use-after-deletion bug as the OP experienced.
#5106 is another bug with the caching system. It doesn't free up m_instrumentView correctly on closing.

[JohannesLorenz]

And there was one further bug introduced by variable-sized instrument views, failing to resize with the arrow key (mostly for Lv2): #5115 . It would be cool if you could check if your fix fixed this, too, and if it helps you decide whether to remove or fix the caching system.

[JohannesLorenz]

@PhysSong you mentioned to have it fixed. Can you please push a PR so I can check if it's suited?

[PhysSong]

I found a bug while testing, so I need to fix it first.

[PhysSong]

While investigating the regression in my fix, I found that a correct fix will introduce too many changes for the stable branch. Is it okay to remilestone this for 1.3?

[Spekular]

No objection here, I imagine very few users will encounter this crash. Those who do can quite easily avoid triggering it again, so I think the total impact of this bug is very low.

[JohannesLorenz]

I also agree. It's a seldom crash, and even if it crashes, you'll hopefully have an autosave. Remilestoning.

[PhysSong]

#5743 is also related to the caching system. It seems like I have a local branch which removes the "misbehaving" caching system, but I stopped working on it while thinking about "what is the best way to fix a minor crash?". I'll check the branch again.