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 #92331: following chord deleted after voice change #2347

Merged
merged 1 commit into from
Jan 27, 2016

Conversation

MarcSabatella
Copy link
Contributor

See my comments in https://musescore.org/en/node/92331#comment-416681. I might be confused about the actual purpose of expandVoice() and the various makeGap functions, so I am trying to be explicit here in my comments. There the cause of the bug is that expandVoice() is supposed to fill a gap at the current segment as well as preceding the current segment, and that makeGapVoice() is correct to assume there isn't already a gap present. expandVoice() was sometimes filling a gap at the current segment, sometimes not - depending on whethere there was also a gap preceding the segment or not. I have taken out the two lines that prevented expandVoice() from filling a gap at the current segment in the case where there was no gap preceding this segment, and this fixes the reported bug. So far, I don't see any ill effects.

I have also added comments to make the function of expandVoice() more explicit. Again, these are based on my assumptions about the purpose of the function.

@lasconic
Copy link
Contributor

It looks correct to me.

I guess it would be good to write some tests for expandVoice() or the callers... any idea?

lasconic added a commit that referenced this pull request Jan 27, 2016
fix #92331: following chord deleted after voice change
@lasconic lasconic merged commit dbd9e84 into musescore:master Jan 27, 2016
@MarcSabatella
Copy link
Contributor Author

If my understanding of the function is correct, I could probably write an mtest just for expandVoice(), to see if it does what I expect in a variety of cases. Maybe a separate one for makeGap(). Between the two of them, they are called a lot - well, maybe only a few direct call sites, but a lot of different paths to reach them. So we are already doing some tests by testing those commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants