Skip to content
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 saving of multiple TempoSyncKnobModels #3281

Merged
merged 2 commits into from Feb 14, 2017

Conversation

@zonkmachine
Copy link
Member

zonkmachine commented Jan 21, 2017

Fixes #3280

@@ -901,6 +901,15 @@ void DataFile::upgrade_1_1_91()
}



void DataFile::upgrade_1_2_0_rc2()

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Jan 21, 2017

Author Member

What should this be? DataFile::upgrade_1_2_0_rc3() ?

@zonkmachine zonkmachine force-pushed the zonkmachine:temposyncknobmodel branch from f5758c3 to 276fc35 Jan 21, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 21, 2017

Fixed. I put the upgrade path in TempoSyncKnobModel::loadSettings() as it would have grown in complexity otherwise.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 24, 2017

I'll merge this in a day or so if there are no objections.

@jasp00
Copy link
Member

jasp00 commented Jan 24, 2017

I put the upgrade path in TempoSyncKnobModel::loadSettings() as it would have grown in complexity otherwise.

What do you mean? loadSettings() has grown in complexity. We are supposed to move away from these patterns. Upgrades belong to DataFile.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 25, 2017

What do you mean? ...

I was a bit tired and got lost in translation. I meant that using DataFile.cpp would be, in this case, much more difficult.
How do you pair syncmode with it's knob? You need to iterate through the list elements and if you find syncmode you know that at least one of the elements is a sync knob but which?

Monstro save file, lmms-1.1.3

monstro o1pw="50" v1e2="0" e2sus="1" l1att_numerator="4" e2rel="0" l1wav="0" e2dec_denominator="4"
s3e1="0" o3vol="33" s3e2="0" w1l1="0" o2spo="0" w1l2="0" o2crs="0" o1ssf="0" o1pan="0" p3l1="0" o2wav="0"
p3l2="0" l2rat_denominator="4" o2ftl="0" e1dec_numerator="4" e2hol_denominator="4" p2l1="0" p2l2="0"
o3wav1="0" o2syn="0" l1phs="0" o3wav2="0" o2ftr="0" o1ssr="0" p1l1="0" p1l2="0" w1e1="0" e2hol_numerator="4"
w1e2="0" l1rat="1" l1rat_numerator="4" o2synr="0" p3e1="0" p3e2="0" e1att_numerator="4"
e1dec_denominator="4" l2wav="0" p2e1="0" p2e2="0" e1dec="0" e1rel_denominator="4" l1att="1714" o3spo="0"
p1e1="0" o3crs="0" p1e2="0" e2pre_numerator="4" e1pre="0" o2pan="0" e1hol_denominator="4"
syncmode="0" l2att_denominator="4" e1att="0" e1hol="0" o3syn="0" f3l1="0" l2att_numerator="4" l2phs="0"
f3l2="0" e2rel_numerator="4" f2l1="0" f2l2="0" e1pre_denominator="4" l2rat="1" e2att_denominator="4"
o3synr="0" e2rel_denominator="4" o1vol="33" f1l1="0" f1l2="0" e1slo="0" e2dec_numerator="4" e2dec="0"
f3e1="0" f3e2="0" l2att="429" f2e1="0" e2pre="0" f2e2="0" o3pan="0" o23mo="0" l1att_denominator="4" f1e1="0"
e2att="0" f1e2="0" e2hol="0" e1sus="1" e2pre_denominator="4" v3l1="0" v3l2="0" e2att_numerator="4" e1rel="0"
e1att_denominator="4" v2l1="0" v2l2="0" l2rat_numerator="4" e1hol_numerator="4" o2vol="33" v1l1="0" o1spo="0"
v1l2="0" o1crs="0" e1pre_numerator="4" e2slo="0" o3sub="0" v3e1="0" v3e2="0" o1ftl="0" v2e1="0" v2e2="0"
s3l1="0" o1ftr="0" s3l2="0" l1rat_denominator="4" e1rel_numerator="4" v1e1="0"

Monstro save file, lmms-1.2.0-temposyncknobmodel branch

monstro e1slo="0" e2pre_numerator="4" e2dec_syncmode="3" s3e1="0" s3e2="0" o3vol="33"
e1rel_numerator="4" l1rat_syncmode="3" o2vol="33" l2att_syncmode="3" e2hol="857.143" o3spo="0"
e2hol_syncmode="3" l1att_numerator="4" o1vol="33" e1hol="857.143" o2spo="0" o3pan="0" p3e1="0" p3e2="0"
p2e1="0" w1l1="0" p2e2="0" p1e1="0" p1e2="0" w1l2="0" o1pw="50" e1dec_numerator="4" e2pre="857.143"
o23mo="0" o1spo="0" l2wav="0" o2pan="0" o2wav="0" l2rat="857.143" e1att_numerator="4" f3l1="0" f3l2="0"
f2l1="0" v3l1="0" f2l2="0" f1l1="0" v3l2="0" v2l1="0" f1l2="0" v2l2="0" v1l1="0" v1l2="0" l1att_denominator="4"
e1dec_denominator="4" e1pre="857.143" l1rat_denominator="4" l1wav="0" e1att_denominator="4" o1pan="0"
l1rat="857.143" o3crs="0" e2rel_numerator="4" e1pre_syncmode="3" e1pre_denominator="4"
e2rel_syncmode="3" o3sub="0" o2crs="0" o1ssf="0" l2att_numerator="4" o1ssr="0" o1crs="0" e2rel="857.143"
e1rel_denominator="4" s3l1="0" s3l2="0" e2sus="1" o2ftl="0" e2dec="857.143" e2att="857.143"
e2dec_numerator="4" o2ftr="0" l2att="857.143" e2att_numerator="4" e1rel="857.143" l1rat_numerator="4"
e1dec="857.143" e1sus="1" o1ftl="0" e1hol_numerator="4" e1att="857.143" l2att_denominator="4" o1ftr="0"
l2rat_denominator="4" e2dec_denominator="4" l1att="857.143" e2att_denominator="4" e1hol_denominator="4"
e1att_syncmode="3" e2pre_denominator="4" p3l1="0" l2rat_syncmode="3" p3l2="0" p2l1="0" p2l2="0"
p1l1="0" p1l2="0" o2synr="0" o3syn="0" o3synr="0" l2phs="0" e2rel_denominator="4" e1dec_syncmode="3"
e1pre_numerator="4" o2syn="0" l1phs="0" o3wav1="0" o3wav2="0" l2rat_numerator="4" e2hol_numerator="4"
w1e1="0" w1e2="0" e1rel_syncmode="3" f3e1="0" f3e2="0" f2e1="0" v3e1="0" l1att_syncmode="3" f2e2="0"
f1e1="0" v3e2="0" v2e1="0" f1e2="0" v2e2="0" v1e1="0" v1e2="0" e2hol_denominator="4" e2att_syncmode="3"
e1hol_syncmode="3" e2slo="0" e2pre_syncmode="3"

@jasp00
Copy link
Member

jasp00 commented Jan 26, 2017

if you find syncmode you know that at least one of the elements is a sync knob but which?

All the ones that have a meter specification. The upgrade step would add a name_syncmode for every name_numerator.

@zonkmachine zonkmachine force-pushed the zonkmachine:temposyncknobmodel branch from 276fc35 to d386d4b Jan 28, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 28, 2017

Thanks!

Getting there slowly... 🐌
I'm pushing a half finished upgrade function for inspection.
I need to find elements that contain "syncmode".
If I've got this anywhere near correct I think the way to proceed from here now it to modify the function void findIds(const QDomElement& elem, QList<jo_id_t>& idList) in DataFile.cpp
to take a string argument and make it generally hand back lists of elements when the node containing that element. It's now hardwired to "id".
https://github.com/LMMS/lmms/blob/master/src/core/DataFile.cpp#L1088:L1100

QDomNode node;

// syncmode for multiple tempo sync knobs
list = elementsByTagName( "syncmode" );

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Jan 28, 2017

Author Member

So, this isn't working. The function I'm looking for would look something like:
list = elementThatHasAttribute( "syncmode" );

@@ -125,6 +125,7 @@ class EXPORT DataFile : public QDomDocument
void upgrade_1_0_99();
void upgrade_1_1_0();
void upgrade_1_1_91();
void upgrade_1_2_0_rc2_23();

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Jan 28, 2017

Author Member

What's a better name for the next upgrade function?

{
name = name.remove( "_numerator" );
name = name.append( "_syncmode" );
el.setAttribute( name, syncmode );

This comment has been minimized.

Copy link
@zonkmachine

zonkmachine Jan 28, 2017

Author Member

This should rather set only the last attribute "syncmode" in every element as only the last one get's saved currently. The other attributes wouldn't get written over as they are today.

@zonkmachine zonkmachine force-pushed the zonkmachine:temposyncknobmodel branch from d386d4b to 873d6c4 Feb 3, 2017
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 3, 2017

@LMMS/developers
Unfortunately I'm not going to be able to solve this within a reasonable time limit. I'm in fact well beyond that limit at this point. Not without a level of support that makes it better for someone else to do it.

I revert to my original working commit and leave the best attempts at upgrade paths (incomplete) as gists. I think probably attempt 2 is the way to go...
temposyncbug upgrade attempt 1
temposyncbug upgrade attempt 2

@jasp00
Copy link
Member

jasp00 commented Feb 4, 2017

Upgrade methods should be run until the build version they were introduced. Pull requests can only estimate this version. A maintenance task should take care of this problem; I know how it should work.

Copy link
Member Author

zonkmachine left a comment

I've tested this and it works very well.

@tresf
Copy link
Member

tresf commented Feb 5, 2017

Upgrade methods should be run until the build version they were introduced. Pull requests can only estimate this version. A maintenance task should take care of this problem; I know how it should work.

Please no.

@jasp00
Copy link
Member

jasp00 commented Feb 5, 2017

Please no.

Why not? We can easily allow upgrades from development versions, without implying official support.
(Do I open a separate issue?)

@tresf
Copy link
Member

tresf commented Feb 5, 2017

It's overkill. We only need to upgrade with RCs at the most granular of levels. We don't release any more granular than RCs, it's plenty.

@jasp00 jasp00 force-pushed the zonkmachine:temposyncknobmodel branch from 94e077c to bce2b5e Feb 13, 2017
@jasp00
Copy link
Member

jasp00 commented Feb 13, 2017

This still works. I will merge in some hours.

@jasp00 jasp00 merged commit c1321ba into LMMS:master Feb 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine deleted the zonkmachine:temposyncknobmodel branch Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.