-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 #299829: Implement extra navigation shortcuts (Top/Trailing/Next-Prev System) #5620
fix #299829: Implement extra navigation shortcuts (Top/Trailing/Next-Prev System) #5620
Conversation
libmscore/cmd.cpp
Outdated
// no chord/rest found? look for another type of element, | ||
// but commands [empty-trailing-measure] and [top-staff] don't | ||
// necessarily need an active selection for appropriate functioning | ||
if (cr == 0 && cmd != "empty-trailing-measure" && cmd != "top-staff") { |
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.
the proposed top and trailing measure functions here have been given functionality without needing a selection, yet still operate within the framework of Score::move() where move selections occur (which is where these commands fit right in), so i allow them to fall through even if no valid chord rest was found here
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.
I'd rather use if (!cr && ...
here
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.
So would I
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.
There were a handful of cr == 0 logical if statements in [cmd.cpp], so in following @Jojo-Schmitz' comment of using !cr I changed all instances to !cr for conformity's sake.
libmscore/cmd.cpp
Outdated
break; | ||
firstMeasure = m; | ||
} | ||
deleteMeasures(firstMeasure, lastMeasure); |
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.
I noticed the cmdRemoveEmptyTrailingMeasures ( Thanks Marc for the pointer which led to attempt having this functionality in the first place ) was a little sloppy here:
already have access to score in Score:: so don't need masterScore() call, and there's a double statement of firstMeasure = lastMeasure right before doing the same thing in the for loop that doesn't actually use for loop's facilities. Because I needed most of the code to get a "receive first trailing measure" and didn't want to have code duplication, I redid this to clear it up with the new firstTrailingMeasure() function and it interfaces much more cleanly.
mscore/scoreview.cpp
Outdated
@@ -1990,6 +1994,8 @@ void ScoreView::cmd(const char* s) | |||
} | |||
else { | |||
Element* ele = _score->move(cmd); | |||
if (cmd == "empty-trailing-measure") | |||
changeState(ViewState::NOTE_ENTRY); |
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.
Again, it made the most sense to enter into NoteEntry after doing this command because it is for the explicit purpose of going to an empty measure, and the odds are that this is a preparation for continuing note entry.
libmscore/cmd.cpp
Outdated
deleteMeasures(firstMeasure, lastMeasure); | ||
auto beginMeasure = firstTrailingMeasure(); | ||
if (beginMeasure) | ||
deleteMeasures(beginMeasure, lastMeasure()); |
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.
At first this looks like I omit testing lastMeasure() validity, but within the firstTrailingMeasure() call, it indirectly verifies lastMeasure();
} | ||
return firstMeasure; | ||
} | ||
|
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.
The fun with firstTrailingMeasure() was having it double as a function for getting the score's first trailing measure, and also having it serve as setting the appropriate rest for a specific staff if there was a valid selection from the caller (Score::move()). I used a default nullptr rather than function overloading, which required a double pointer rather than a reference since references can't be null
One good use case for [Go to next/previous system] is when making exercises. These often fit on a single line, so it also means [Go to next/previous exercise] |
But if an exercise is not a single line, then that's misleading. Better probably to use section breaks to mark exercises, and implement a "go to next section" command. |
Right. It "could" mean that, but not always. Another thing that comes to mind is that sometimes exercises, when short enough, can be separated into two or more 'sections' on the same line with a horizontal frame, and [go to next/previous system] would skip right over that since measures after a horizontal frame aren't considered to be on a separate system. Although, a [go to next section] could be definitely useful depending on the type of score (particularly big scores that deal with many exercises like chorales or etude collections, etc). Then again, there's already the "Find" function that finds, among other things, Rehearsal Marks. Rehearsal Marks can be visible or hidden depending on the way the score should look, and then searched for quickly as a sort of workaround for a "go to next section", but of course not as easily as a shortcut command could be. |
FWIW, I use rehearsal marks to number t exercises in the accessible teaching materials I create, allowing a blind student to go directly to any exercise by number, which has been extremely useful. So a “jump to next rehearsal mark” command would actually be very much in keeping with this. |
961379b
to
a9e7b3a
Compare
0fa3a06
to
e26350e
Compare
I manually merged the changes from this PR to 3.x. Could you please rebase this PR to the latest master? |
Manually merged #5620 to 3.x
e26350e
to
6d5461b
Compare
bc77215
to
ac9e6ca
Compare
Resolves: The suggestion to implement extra navigation shortcuts: https://musescore.org/en/node/299829
Originally, this was for [go to next/previous system] (#5596) which brought the selection cursor to the top-left of the next/previous system. After the hint that this was too specialized and that other commands should be provided instead/additionally, here is a group of four commands and a description of their behavior. I will cancel the previous pull request since this supersedes it.
[Go to top staff] - Goes to current measure's top staff. If there is no selection at all, this will go to the top staff at measure one
[Go to empty trailing measure] - This is a little more interesting. If there is no active selection, the result will be to take the user to the first staff at the first empty trailing measure of the entire score, practically the same position where the Tools->Remove Empty Trailing Measures begins deleting. This will give the exact same desired activity i wanted in the old proposed [next system] behavior, under the assumption that the user is working per-system and that the next system starts at the empty trailing measure.
To give this command a double-edged sword, this will also go to an individual staff's empty trailing measure which may or may not be equivalent to the entire score's empty trailing measure. This can be also useful at times, and at first it was thought that only one or the other would be able to be implemented as a trade off, but after testing, I think having both function and contingent upon whether or not there's a selection makes the most sense with the only trade off being that if the user mainly wants the entire score's trailing measure instead of an individual staff's trailing measure, an [Escape] or clicking off of any elements would be required first. P.S. Since this is dealing explicitly with going to an empty measure, it seemed most logical to enter into Note-Entry Mode automatically after issuing this command. Not so with the other ones.
Finally, although falling into the background, I didn't want to completely banish having a pair of:
[Go to next/previous system] commands to have available if a user should so happen to find it useful (I still do at times even with the above two other functions) depending on their workflow (Especially for score traversal in larger scores). However, this time, the selection will continue on the current working staff as would be expected for default behavior. So here the user can traverse per system forward, or backward. Backward will also take the user to the beginning of the current system before going to previous system. These are moreso 'helper' functions like with the [go to top staff] in that if set up by a user, they can aid in speed (instead of doing ctrl+left/right multiple times or alt+up multiple times, these could be used).