-
Notifications
You must be signed in to change notification settings - Fork 3k
Allow T on list selection #31102
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
base: master
Are you sure you want to change the base?
Allow T on list selection #31102
Conversation
89a5b2a to
be62a67
Compare
src/engraving/editing/edit.cpp
Outdated
| if (!tieNote && selection().isList() && sameChord) { | ||
| Tie* tie = nullptr; | ||
|
|
||
| if (sameChord) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking sameChord twice
And tie seems an unnecessary variable
It would be nice to move this outside for (size_t i = 0;… loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
be62a67 to
03a3056
Compare
b7d3101 to
cd5a17a
Compare
| std::vector<Note*> tieNoteList(notes); | ||
| const bool shouldTieListSelection = notes >= 2; | ||
| Note* tieNote = nullptr; | ||
| Note* n = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n is only used for getting the Chord. So instead of extending the scope of n, let's add a Chord* chord variable, that's initialised as noteList[0]->chord(); then you can reuse that variable in the std::all_of predicate.
| } | ||
| } | ||
|
|
||
| if (!tieNote && selection().isList() && sameChord) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the logic has become the following:
if for the last selected note no tieable note was found, then cmdAddTie.
But I suppose the intention was:
if for any selected note [...].
I must say the meaning of the condition is not immediately clear in the way it's written right now. Is it supposed to be "If all selected notes are in the same chord, and for any one of them there is no existing note to tie to, then add new tied notes"? It might help to add a comment or introduce a descriptively named variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! I shouId've made it clearer, but I intentionally left the condition a bit loose so we could discuss and agree on the ideal behavior before finalizing it. Once we settle on what makes the most sense, I can go ahead and refine the logic and add clarity through comments or better variable naming.
edit: The original behavior sent (i.e with the block inside the iteration loop) was: "If all selected notes are in the same chord, and for any one of them there is no existing note to tie to, then add new tied notes overwriting the next Chord"
Resolves: #27905
Allows T to work on list selections outside note entry. Works only on single chord selections as shown below.
(Would be cool to add multiple staves support later on)
Recording.2025-11-18.221523.mp4
Another option would be to add a tied note to the next chord without overwriting.
Third option would be to only allow cases where next chordRest is rest only. (see discussion in #27905)