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 #62961: multiple hairpins added on list selection #2042

Merged
merged 1 commit into from
Jun 4, 2015

Conversation

MarcSabatella
Copy link
Contributor

Turns out it isn't straightforeard to just add a single hairpin to each note. cmdAddHairpin tries to find appropriate notes within the selection; it isn't actually passed the current note. We could add that as a parameter, but then it still wouldn't be clear when to use the current method and when to use the passed note. It might be nice to support a single note selected on each staff, but since we support ranges across staves, that should be good enough most of the time.

@lasconic
Copy link
Contributor

lasconic commented Jun 4, 2015

mm this is a pity. If I'm not wrong, it means that selecting a note and another note further on the same staff and pressing the crescendo shortcut will not work anymore.

@lasconic
Copy link
Contributor

lasconic commented Jun 4, 2015

I tried a couple of other things like clearing the selection or removing cr1 from the selection but it's not really good. We lack the information that it's a call made several time.
Another solution could be to disallow the addition of a hairpin at the same tick/tick2 and track. In most case, it means the two hairpins are on top of each other anyway. Sibelius does allow this but draw the second hairpin in red. (the third one hide it though..)
Last solution, we could differentiate a single call from the shortcut, from a (potentially multiple) call from drag.

@lasconic
Copy link
Contributor

lasconic commented Jun 4, 2015

I merge this for now.

lasconic added a commit that referenced this pull request Jun 4, 2015
fix #62961: multiple hairpins added on list selection
@lasconic lasconic merged commit 10a72eb into musescore:master Jun 4, 2015
@MarcSabatella
Copy link
Contributor Author

FWIW, it didn't really work correctly to select two notes then apply the hairpin - it triggered the bug being fixed here. That is, you got two overlapping hairpins.

I'm sure it would be possible to fix this better with a more substantive change to how the code is structured. I thought about changing the code upstream where we loop through the notes applying the drop. We do already special case slurs and hairpins for range selections to prevent exactly this problem - we only apply to one note one each staff. We could do something similar here, and then also make sure Note::drop() passes in "this" so cmdAddHairpin knows what is being applied to.

@lasconic
Copy link
Contributor

lasconic commented Jun 4, 2015

It was working to select two notes and use the shortcut since the cmdAddHairpin function is called only once.

@MarcSabatella
Copy link
Contributor Author

Yes, true.

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