Restore the track height when loading files

[michaelgregorius]

The track height is already stored for every file that is saved. Remove a
condition that prevents its retrieval from files.

The removed condition was added as a fix for an issue with the number
3585927 but it was not possible anymore to find out what this issue was
about.

[michaelgregorius]

@LMMS/developers Does anyone know/remember what 3585927 was about? Or where to find information about the bug? @tobydox perhaps?

If we cannot find out what it was about I propose to merge this one in the next few days. If we find a problem with the change later we will either fix the problem or revert the commit.

[tresf]

@michaelgregorius here's the commit that introduced it d3d6d65

[zonkmachine]

I tested #3692 and it works fine. As for 3585927 I had only limited success with wayback machine. You can only see page one from every backup occasion but some bugs will show up, in this case just the title of it.

3585927 | Track Gets Huge Vertical Enlargement; Notes Randomly Vanish

[michaelgregorius]

Rebased onto the current master (with the changes merged from 1.2). If no one objects I will merge this one soon. Seems like the secret of 3585927 will be lost forever. 😉

[zonkmachine]

Track Gets Huge Vertical Enlargement; Notes Randomly Vanish

I'm still waiting for notes to randomly vanish... 😃
The patterns that turn up this large all seem to have the same height, height="467", so the data seem to get corrupted on saving. So far I have only seen this with projects done before ebc9137.

[zonkmachine]

The track gets the height of the instrument... sometimes. We simply have to get used to it.

[michaelgregorius]

@zonkmachine Do you have some steps or an example file with which this behavior can be reproduced?

[zonkmachine]

No. I've tried but can't reproduce it on will. I'll try a bisect.

[zonkmachine]

Bug introduced here: 69947a6

commit 69947a624b43a5de257217b3af9e1383312ba305
Author: Raine M. Ekman <raine@iki.fi>
Date:   Thu Nov 1 21:35:17 2012 +0100

    Track: allow smaller height
    
    Here's one way to cram more stuff onto small screens, or otherwise help
    reducing visual clutter: Allow tracks to be shift-dragged all the way
    down to 8 px height.

[michaelgregorius]

@zonkmachine Good catch! Then I guess it's lines like this one that somehow create problems with tracks that have been saved with the smaller possible heights before the value for the minimum height got increased again.

[zonkmachine]

that have been saved with the smaller possible heights before the value for the minimum height got increased again.

But the tracks that are wonky are saved with the wrong big size already. height="467"
It's the same size as the full height of the instrument window. The only place I find m_height is given a value that isn't a literal is here: https://github.com/LMMS/lmms/blob/master/src/core/Track.cpp#L2831
I think height() somehow is referencing the instrument window instead of the track, if that makes sense.

We both need a fix for the bug and an upgrade path to set all nutty tracks to default.

[zonkmachine]

if( storedHeight >= MINIMAL_TRACK_HEIGHT && storedHeight < 400 )

This works as a quick dirty fix.

[zonkmachine]

MAXIMUM_TRACK_HEIGHT ≈ 200 ?

[michaelgregorius]

@zonkmachine I think we should not add some conditional code again as this would confuse users who want to set one or more tracks to a very large height. If these users save their project and then load it again the height would not be the same as it was when it was saved.

Can the saving with a height of 467 also be produced with current versions? If it was only a certain (released) version that produced these files we can check for this version during the load and correct the heights.

Another interesting question is whether these corrupted files have been saved with an officially released version or with developer versions. If these files were produced using developer versions I would not care. In this case we should consider to just load and save the heights as they are and if a user has a corrupted file he/she can correct the heights and save again.

[zonkmachine]

I think we should not add some conditional code again as this would confuse users who want to set one or more tracks to a very large height. If these users save their project and then load it again the height would not be the same as it was when it was saved.

if ( storedHeight == 467 ){ storedHeight = DEFAULT_TRACK_HEIGHT }

I don't intend it as a fix but as to temporary contain the bug as it's annoying as hell. Not everybody needs to share the suffering.

$ grep -Ro height=\"467\" ~/lmms/projects/ | wc -l
60

So I have 60 cases and these are the projects that are uncompressed only.

Can the saving with a height of 467 also be produced with current versions?

Yes, the first one I saw was from a recent version. It's tempting enough to put an assert in there but the only place I don't want a crash is when saving. I think I'll test an assert anyway.

PS. I think the bisect results are probably incorrect. I need to look into that further.

[michaelgregorius]

@zonkmachine I think I have gotten a bit closer to the cause of the problem. First of all I'm also able to reproduce the problem with a fresh file and a current version of the software. For my test case I have the following tracks:

The problems occurs with the following steps:

1. Resize the track of the first (upper most) Triple Osc slightly.
2. Open the instrument window of the first Triple Osc.
3. Use the left arrow key in the instrument window twice to go to the instrument window of the Organic synth.
4. Use the right arrow twice to go back to the first Triple Osc.
5. Save the file.
6. Open the file. The track is now too large.

The first thing that I did to further investigate was to set a conditional break point in Track::setHeight. This break point was set to trigger when the height is larger than 400. However, it was never triggered and the break point in the saving code in Track::saveSettings also never showed an unusually large value.

Then it dawned on me that perhaps some other class writes the faulty values into the XML. A quick search for "height" only found one other place in MainWindow::saveWidgetState where this string is used. I altered the code a bit to be able to break for heights larger than 400. This one is now in fact triggered and also with cases where the height has a value of 472 which is the problematic value in my tests. The next entry on the call stack is InstrumentTrackWindow::saveSettings in this case and I guess this is where the height of the instrument window comes from.

[zonkmachine]

Another method.

1. Empty project.
2. Add any amount of instruments.
3. Switch instuments as above any amount of steps and any direction.
4. Save.
5. The last selected instrument will come back with a 'big ass' height.

[michaelgregorius]

Some more debugging results: if you want to know why the switching of the instruments is relevant for the bug set a break point in SerializingObject::setHook. The switching of instruments at one point calls InstrumentTrackView::getInstrumentTrackWindow which installs a hook for the serialization so that InstrumentTrackWindow::saveSettings is called as well.

When the instrument track is saved via Track::saveSettings the original tag name of the DOM element that the data is saved into is instrumenttrack. However, because m_simpleSerializingMode is set to false this tag name is changed to track. The parent of the element is trackcontainer. This means that we save the height of the track itself to the attribute height of the element trackcontainer > track.

Then InstrumentTrackWindow::saveSettings is called. It delegates the storage of the window geometry to MainWindow::saveWidgetState. The element that is given into that method also is a DOM element track under trackcontainer. This means that the height of the window geometry is stored in the attribute height of the element trackcontainer > track as well.

This means that the attribute height in trackcontainer > track is used to describe the height of the instrument window as well as the height of the track in the track list which leads to the bug.

To solve this problem we will first have to clarify whether the two sets of data described above are saved in the correct place. It would also be helpful to understand what m_simpleSerializingMode does and why it is needed in the first place.

[zonkmachine]

If you can find a way to trigger this without the instrument switch buttons, which was introduced rather recently, I can redo the bisect and get a better result.

This bug is ever as amazing as I thought it would be when I first saw the title of it. It just keeps on giving...

[zonkmachine]

To solve this problem we will first have to clarify whether the two sets of data described above are saved in the correct place. It would also be helpful to understand what m_simpleSerializingMode does and why it is needed in the first place.

As for your latest report it is a bit beyond me but simpleSerializingMode was introduced here.

[michaelgregorius]

@zonkmachine I have created #3721 to cover the bug because this pull request is already closed and therefore invisible.