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 #297423: allow ctrl+click to modify range selection #6139

Merged
merged 1 commit into from
Jun 6, 2020

Conversation

MarcSabatella
Copy link
Contributor

This partially resolves https://musescore.org/en/node/297423,
and addresses a common request:
the ability to add or subtract an element from a range selection.
We currently allow building list selections with ctrl+click,
but ctrl+click does nothing interesting with a range selected.
While some might want a new "magic" range selection
that has random elements added or removed from it,
that is beyond the scope of what I am doing here.
This change simply converts the range to a list
before processing the ctrl+click.
So the end result is a list selection that consists of
everything that was formerly in the range selection,
plus or minus what you just ctrl+clicked.

This partially resolves https://musescore.org/en/node/297423,
and address a common request:
the ability to add or subtract an element from a range selection.
We currently allow building list selections with ctrl+click,
but ctrl+click does nothing interesting with a range selected.
While some might want a new "magic" range selection
that has random elements added or removed from it,
that is beyond the scope of what I am doing here.
This change simply converts the range to a list
before processing the ctrl+click.
So the end result is a list selection that consists of
everything that was formerly in the range selection,
plus or minus what you just ctrl+clicked.
@@ -399,6 +399,11 @@ void ScoreView::mousePressEventNormal(QMouseEvent* ev)
}
else {
if (st == SelectType::ADD) {
// convert range to list
if (e->score()->selection().isRange()) {
e->score()->selection().setState(SelState::LIST);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has an effect on all operations acting differently on the selection being a list or a range, e.g. Palette::applyPaletteElement().
Selecting a range and double click on a double barline, the barline at the end of the measure is changed. With this PR, a double barline is created just before all selected ChordRest's.
If only one ChordRest is selected, the double barline is create again at the end of the measure.

Copy link
Contributor Author

@MarcSabatella MarcSabatella May 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following. This function isn't called when applying palette elements, only when actually clicking within the score. And the change here only applies to Ctrl+click. Unless there is something I'm missing? Were you able to duplicate a problem? I am not able to, but maybe I am misunderstanding the steps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I can't reproduce it myself now. When I tried yesterday I got a very weird situation. I think it was related of a list which was, using this patch, converted into a range. And the problem is Palette::applyPaletteElement() handles the dropped element differently whether the selection is a list or a range.
Sorry, most likely not an issue at all, just confused with the different behavior of lists and ranges by Palette::applyPaletteElement().

@MarcSabatella
Copy link
Contributor Author

I have a pretty good idea now of what happened. After selecting the range, you first did a ctrl+click on some element within the score, which converted the range to a list exactly as this PR is designed to do. Then you tried adding a barline using the palette to the list selection. My PR doesn't change this behavior one bit. Applying palette items to list selections does indeed apply them to each element in the list, by design, except for a few cases handled specially like slur.

All this PR does is make it easier to create certain types of list selections - ones consisting of everything in a range except a handful of specific elements, or everything in a range plus a handful of specific elements.

@njvdberg
Copy link
Contributor

That happened indeed. And to make it more complicated, in my example the selection also contained the first note of a measure and the barline was created before all selected notes, except of course the very first note of the measure. Just having a bad example made it confusing.
But this PR, together with its friends certainly makes the selection more easy.

@anatoly-os
Copy link
Contributor

I wouldn't apply these changes to 3.x, but to master. The change sounds like something that might unexpectedly change user experience.

@MarcSabatella
Copy link
Contributor Author

I can't say that I care a lot about this one. But, I don't think there is anything to worry about here, either. Ctrl+click does nothing reasonable at all right now if a range selection exists. Ctrl+click on an element within the range has no effect whatsoever. Ctrl+click on an element outside the range clears the selection. I can't see that anyone would be relying on Ctrl+select to do either of these things. About the only reason a user might do Ctrl+click with a range selected is that he wants something like like this PR to be the result.

Of course, he might actually want the result to still be a range somehow, but as mentioned, that's a whole other can of worms.

@njvdberg
Copy link
Contributor

njvdberg commented Jun 3, 2020

The change sounds like something that might unexpectedly change user experience.

I think this is because of my remarks 😞, it was just my confusion, combined with unlucky example. These three related PR's (#6137, #6138, #6139) will give a more consisted experience and it would nice if all three of them could be part of 3.5.

@anatoly-os anatoly-os merged commit a99db6e into musescore:3.x Jun 6, 2020
anatoly-os added a commit that referenced this pull request Aug 7, 2020
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.

3 participants