fix 'Set Linear' not saving properly (#1642) #2742

Merged
merged 3 commits into from Aug 14, 2016

Projects

None yet

4 participants

@serdnab
Contributor
serdnab commented Apr 28, 2016 edited

I added setScaleType( Linear ) when the loading element doesn't have a child element (containing scale type or automation data) as this is the default save way for non automated linear knobs
fixes #1642

@Umcaruje
Member
Umcaruje commented Apr 30, 2016 edited

@serdnab could you please rename the PR to something more descriptive like " Fix 'Set Linear' not saving properly"

It'd be also very good if you renamed your commit too (git commit --amend) and afterwards you'll need to force push (git push -f)

@serdnab serdnab changed the title from fix for #1642 to fix 'Set Linear' not saving properly (#1642) May 1, 2016
@Umcaruje
Member
Umcaruje commented May 1, 2016

Did a quick test, seems to fix #1642 for me. I'm not comfortable with merging this though, without any more extensive testing, because I'm not familiar with this part of the codebase at all.

@Umcaruje
Member

@LMMS/developers could someone else review/test this PR out, so we could merge?

@jasp00
Member
jasp00 commented Jun 12, 2016

Should not the backward compatibility code be in an upgrade method in src/core/DataFile.cpp?

@Umcaruje
Member
Umcaruje commented Jul 5, 2016

@serdnab could you please address the issue @jasp00 pointed out?

@serdnab
Contributor
serdnab commented Jul 9, 2016

Which is your opinion on opening old projects that their origin versions of lmms didn't have logarithmic knobs. Open it linear or as detected by isLogarithmic() in LadspaManager.cpp?

@tresf
Member
tresf commented Jul 9, 2016

Which is your opinion on opening old projects that their origin versions of lmms didn't have logarithmic knobs. Open it linear or as detected by isLogarithmic() in LadspaManager.cpp?

Linear.

@tresf
Member
tresf commented Jul 9, 2016 edited

Also as @jasp00 points out, we want to always preserve the bug for older projects when possible. e.g. if it was incorrectly setting Logarithmic before when Linear was explicitly being clicked, write Logarithmic to the config and make them change it again to take effect. These things can really destroy projects if not upgraded properly.

@serdnab
Contributor
serdnab commented Jul 21, 2016

@Umcaruje

could you please address the issue @jasp00 pointed out?

done

@jasp00
Member
jasp00 commented Jul 21, 2016

@serdnab: This branch has conflicts that must be resolved

@serdnab
Contributor
serdnab commented Jul 22, 2016

@jasp00

This branch has conflicts that must be resolved

solved

@jasp00 jasp00 commented on an outdated diff Jul 23, 2016
include/DataFile.h
void upgrade_1_1_0();
void upgrade_1_1_91();
void upgrade();
void loadData( const QByteArray & _data, const QString & _sourceFile );
+ void findIds(const QDomElement& elem, QList<jo_id_t>& idList);
@jasp00
jasp00 Jul 23, 2016 Member

This function does not depend on the class. It should be a static function in DataFile.cpp.

@jasp00 jasp00 commented on an outdated diff Jul 23, 2016
src/core/DataFile.cpp
+ for(QDomNode node = list.item(i).firstChild(); !node.isNull();
+ node = node.nextSibling())
+ {
+ QDomElement el = node.toElement();
+ QDomNode data_child = el.namedItem("data");
+ if(!data_child.isElement())
+ {
+ if (el.attribute("scale_type") == "log")
+ {
+ QDomElement me = createElement("data");
+ me.setAttribute("value", el.attribute("data"));
+ me.setAttribute("scale_type", "log");
+
+ jo_id_t id;
+ for(jo_id_t tid = rand(); idList.contains(id = tid % EO_ID_MSB
+ | EO_ID_MSB); tid++)
@jasp00
jasp00 Jul 23, 2016 Member

The ids from loaded projects do not have EO_ID_MSB set, that would conflict with new objects. This id allocation from ProjectJournal should not be here; just assign any free id, that is, tid = last_assigned + 1 starting from 0.

Assume tabs are 8 spaces. This line is difficult to see in the web interface.

@serdnab
Contributor
serdnab commented Jul 29, 2016 edited

@jasp00

addressed your requests

@jasp00 jasp00 commented on an outdated diff Aug 2, 2016
src/core/DataFile.cpp
+ for(QDomNode node = list.item(i).firstChild(); !node.isNull();
+ node = node.nextSibling())
+ {
+ QDomElement el = node.toElement();
+ QDomNode data_child = el.namedItem("data");
+ if(!data_child.isElement())
+ {
+ if (el.attribute("scale_type") == "log")
+ {
+ QDomElement me = createElement("data");
+ me.setAttribute("value", el.attribute("data"));
+ me.setAttribute("scale_type", "log");
+
+ jo_id_t id;
+ for(jo_id_t tid = last_assigned_id + 1;
+ idList.contains(id = tid); tid++)
@jasp00
jasp00 Aug 2, 2016 Member

id should be used directly in the loop, tid is not needed.

@serdnab
Contributor
serdnab commented Aug 10, 2016

@jasp00

done
deleted the unnecessary variable

@jasp00 jasp00 commented on the diff Aug 14, 2016
src/core/DataFile.cpp
@@ -794,6 +797,47 @@ void DataFile::upgrade_0_4_0_rc2()
}
+void DataFile::upgrade_1_0_99()
+{
+ jo_id_t last_assigned_id = 0;
+
@jasp00
jasp00 Aug 14, 2016 Member

Empty lines should not have any indentation, but coding conventions are not against this.

@jasp00
Member
jasp00 commented Aug 14, 2016

Current projects do not need a manual upgrade in this case.

@jasp00 jasp00 merged commit 07021ed into LMMS:master Aug 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment