fix #290121: changes to non-expr patches aren't saved #5094
Conversation
|
some mtests are not happy |
jthistle
force-pushed
the
290121-patch-changes-not-saved
branch
2 times, most recently
from
29e5716
to
1f43fa0
Compare
|
Better than before, but still an mtest failure, see https://travis-ci.org/musescore/MuseScore/jobs/540934522#L4243-L4264 |
jthistle
force-pushed
the
290121-patch-changes-not-saved
branch
from
1f43fa0
to
7ba87e1
Compare
|
Should all be fixed now :) |
fix #290147 as well
jthistle
force-pushed
the
290121-patch-changes-not-saved
branch
from
7ba87e1
to
f3d0443
Compare
| if (e.dataA() == CTRL_HBANK && e.dataB() == 0) | ||
| continue; | ||
| if (e.dataA() == CTRL_LBANK && e.dataB() == 0) | ||
| if (e.dataA() == CTRL_LBANK && e.dataB() == 0 && !_userBankController) |
There was a problem hiding this comment.
It looks like this part of the patch (the changes inside if (e.type() == ME_CONTROLLER)) is not really necessary to fix the issue and causes those extra <controller> tags to appear. At least in my tests the scenario from the issue 290121 works correctly without changing this part of the code. Are there any reasons why this change might be necessary to do?
There was a problem hiding this comment.
This is just a reworking of the initial statement, I guess, with the added check for if it is 0 or not. I think it should be alright without these changes then... but I'd need to test it.
There was a problem hiding this comment.
No hang on, we always want to write the bank controller if it was set by the user, even if the data is 0, so that if it's set to 0 from a number of 1, say, it's not automatically switched back to 1 when its read (since if a bank value is read from the file, then we don't automatically switch to expressive patches). So, this line is correct and should stay as it is.
The old line doesn't account for this since if the bank is 0, then it won't be written, even if the user has changed it to that.
There was a problem hiding this comment.
Then it does not agree with the other part of this patch. If you explicitly write only one of HBANK or LBANK controllers, the value of the other will be 0 by default, and _userBankController flag will be set to true, so the effect will be essentially the same. If you explicitly write both of them and their value is 0, then _userBankController flag will be anyway reset to false. So, if I am correct, there are no situations where writing zero values for those controllers makes any difference to the result being obtained on file reading.
There was a problem hiding this comment.
Hmmm... it's worth noting that that other part of the patch is just a slightly hacky fix to get SND working with 114 import. But yes, I think you might be right. I'll change it back.
There was a problem hiding this comment.
Ok, testing it, it actually is important to have it the way it is. With your proposed changes:
- create a score with an expressive instrument
- change it to the non expr version in the mixer
- save and close
- reopen the file
- it's changed back to the expr version.
This happens for the reasons above:
we always want to write the bank controller if it was set by the user, even if the data is 0, so that if it's set to 0 from a number of 1, say, it's not automatically switched back to 1 when its read (since if a bank value is read from the file, then we don't automatically switch to expressive patches
Hope that makes sense, I realise I'm not explaining it the best. tl;dr: it should stay as is.
There was a problem hiding this comment.
Ah, I didn't notice that chunk of code actually belongs to read114. Sorry then, it turns out there is no contradiction here indeed, we indeed are better to write those tags.
There was a problem hiding this comment.
It's fine, it's a really confusing area of the codebase unfortunately. Thanks for merging :)
Resolves: https://musescore.org/en/node/290121