MuseScore icon indicating copy to clipboard operation
MuseScore copied to clipboard

Improvements to text navigation

Open kennysiucho opened this issue 1 year ago • 14 comments

Resolves: #22440

Replicates the behavior of lyrics when navigating via arrow keys for sticking and fingerings.

The issue also mentions potentially navigating using delete/backspace as well - I'd imagine something like when the current sticking/fingering being edited is empty, pressing backspace will go to the previous sticking/fingering and highlight it. If that's something we want, I can also look into implementing it.

Resolves: #24295

This issue arises because calling Score::nextElement on a frame will go to its child text, and doing it again on the child text will go to its parent frame if the frame is the lastElement of the score, resulting in an infinite loop. You can see this when navigating the score using Alt-left/right.

I fixed this by making lastElement return the last child of the frame instead of the frame itself since navigating via Alt-right would end up there, though I'm not sure if that's the best way to fix it. If there are potentially other loops in navigation like this, it would be better to rewrite the logic in NotationInteraction::navigateToNearText.

Resolves: #13148

Alt-left/right while editing staff text, system text, and expressions will now go to the next note and create a new text, which was the behavior in MS3.

  • [x] I signed the CLA
  • [x] The title of the PR describes the problem it addresses
  • [x] Each commit's message describes its purpose and effects, and references the issue it resolves
  • [x] If changes are extensive, there is a sequence of easily reviewable commits
  • [x] The code in the PR follows the coding rules
  • [x] There are no unnecessary changes
  • [x] The code compiles and runs on my machine, preferably after each commit individually
  • [ ] I created a unit test or vtest to verify the changes I made (if applicable)

kennysiucho avatar Sep 13 '24 12:09 kennysiucho

Thank you! Writing sticking is so much easier now! :)

"Replicates the behavior of lyrics when navigating via arrow keys for sticking and fingerings." Just to confirm, it is not possible to move between fingerings with arrow keys?

As this resolves the crash and improves the experience, I'd be in favor of merging this PR asap. Perhaps implement the backspace navigation later.

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved

https://github.com/musescore/MuseScore/issues/22440 FIXED https://github.com/musescore/MuseScore/issues/24295 FIXED

zacjansheski avatar Sep 13 '24 16:09 zacjansheski

@zacjansheski cursor keys have never worked directly for fingering, but what is supposed to work is Alt+Left/Right. Normally this visits all elements in the score, but if you're in text edit mode, it is intended to navigate to the next note to allow more text to be added. It worked for fingering, staff text, and other text types. Unfortunately this broke at 4.0 and hasn't yet been fixed - see https://github.com/musescore/MuseScore/issues/13148. Looks like it has been slated for fixes in 4.2 and then 4.3 and then 4.4 but keeps getting put off.

FWIW, it was a useful but not especially well-known feature. For fingering specifically, Space & Shift+Space were also supported and that is what most people use I assume.

MarcSabatella avatar Sep 13 '24 16:09 MarcSabatella

"Replicates the behavior of lyrics when navigating via arrow keys for sticking and fingerings." Just to confirm, it is not possible to move between fingerings with arrow keys?

Sorry I got myself confused there. Arrows keys don't work for fingerings at the moment, but space and shift-space does now highlight the fingering. However, I was wondering if we would want arrow keys to work for fingerings too - I suppose it serves a similar purpose to sticking, just used more sparsely perhaps. If so, it would just be copy-pasting the code over.

As for Alt-left/right, I managed to get it somewhat working for most types of text, but there are cases like dynamics and tempo markings where it already behaved somewhat strangely in MS3. I also don't think it's particularly helpful to create empty dynamics or tempos for users. Maybe it would be better to restrict Alt-left/right to certain text types - system/staff text, sticking, fingering, lyrics, chord symbols, figured bass, and rehearsal marks work fine I think?

kennysiucho avatar Sep 14 '24 10:09 kennysiucho

In my opinion, staff, system, and expression text are realistically the only types where it is import, since for things like chord symbols or lyrics there is already native navigation facility, and for things like dynamics it doesn’t directly make sense right now (although I’m not sure if the new dynamics facility from the GSoC project will change that).

MarcSabatella avatar Sep 14 '24 12:09 MarcSabatella

I've allowed staff text, system text, and expressions to navigateToNearText instead when alt-left/right is pressed during text editing. When done on other text types, the behavior shouldn't be affected and it will only move the text cursor.

I've also added arrow navigation for fingerings since it ~~had no navigation facilities.~~ also only had space/shift-space navigation

kennysiucho avatar Sep 15 '24 08:09 kennysiucho

This looks great! Thank you for adding the one for fingerings as well.

One request @Nekotizimo: while we're adding the Alt + Left/Right shortcut back in for text elements, it should also work for tempo text and rehearsal marks. From a quick look through our element types I think those are the only other applicable cases.

avvvvve avatar Sep 18 '24 13:09 avvvvve

@avvvvve Thanks! It should be straightforward for rehearsal marks, but what should be the behavior for tempo texts?

  • Text: should it be empty, or should there be a placeholder, like "quarter note = 80" (like what alt-shift-T currently does) or just "quarter note = "? Then if it wasn't modified, when alt-right is pressed again, it deletes the current tempo and moves on to the next one?
  • Tempo property: should it have a default value (like 80bpm), or should it be set to whatever the tempo previously was as to not change the tempo?

In my opinion, it feels weird to be creating tempo texts from scratch with alt-arrow, and it might only be useful for navigating between existing tempo markings. Unlike staff texts etc, you would choose the type of tempo marking you want from the ones in the palette, instead of typing in note symbols. Let me know what you think.

Screenshot 2024-09-18 at 11 18 48 PM

kennysiucho avatar Sep 18 '24 15:09 kennysiucho

Good points, I think it's easy enough to add tempo with the shortcut so I take that back! But if it's a quick fix for rehearsal text let's do it.

I also quite like your idea of having a way to use arrow keys + a modifier to jump to the previous/next instance of the same element type, but I'd want to consider other element types and that feels like a feature request separate from this. In the meantime I'm going to audit what all of our arrow + modifier behaviors are. Thanks!

avvvvve avatar Sep 18 '24 20:09 avvvvve

@Nekotizimo Nearly inconsequential thing: Just noticed that Alt + Spacebar now does not enter a space in system, staff, and expression text, where it used to. I don't know that anyone will ever do this but it's technically a regression if you don't mind fixing that too!

avvvvve avatar Sep 19 '24 13:09 avvvvve

Added alt-left/right navigation for rehearsal marks, and fixed alt-[other keys] not registering.

Should I rebase to fix the failing vtest?

kennysiucho avatar Sep 21 '24 09:09 kennysiucho

@Nekotizimo Apologies for the delay! Noticed a couple things that need fixing:

  1. When you're editing the very first lyric on a staff and press left arrow OR Shift + space, nothing happens and you remain in text editing mode (expected). Doing the same with sticking text, the element gets deselected (and if you press it again, it selects the first note of the first staff of the score), and with a fingering element, selection goes to the first note on that staff. Sticking and fingerings should behave like lyrics in this case.

https://github.com/user-attachments/assets/14251cf2-ea9f-40e9-9880-9814334248b9

  1. Similarly, pressing Alt + left arrow while editing system/staff/expression text and rehearsal marks that are at the first beat of the score takes you out of edit mode, deselecting the element. It should just stay in edit mode.

https://github.com/user-attachments/assets/532e5664-cd78-4dd6-9326-ccc04728967e

  1. When editing system/staff/expression text and rehearsal marks, navigating with Alt + arrow skips over elements that only have a rest on the staff immediately beneath it. For lyrics, fingerings, and stickings, Space and Shift + Space also skip over elements that are on rests. Alt + arrow should target notes and any existing instances of the text type you're editing.

https://github.com/user-attachments/assets/a7f38175-249e-4cca-9792-41e2d91aeb36

avvvvve avatar Oct 02 '24 18:10 avvvvve

What is the current situation with this PR? @Nekotizimo Is it right that you are working on the improvements suggested by Ben?

cbjeukendrup avatar Oct 10 '24 18:10 cbjeukendrup

Sorry I've been busy with uni, but I am working on the changes. Hopefully will be able to get it done in the next few days.

kennysiucho avatar Oct 10 '24 22:10 kennysiucho

Sorry for the delay. The latest suggestions from @bkunda should be implemented now. I've rewritten the navigateToNearText to be more similar to the navigation of lyrics & harmony, i.e. getting prev/next segments instead of using nextElement/prevElement, which requires us to select each of the elements. Otherwise, in order to do nothing when the start/end of the score is reached, we would have to restore the original selection, including the state of the text edit, but it feels like a slightly janky way of approaching it, and I encountered some weird crashes in my attempts. This is my first time writing this much logic involving the score dom so I would appreciate any feedback! @cbjeukendrup

Now, you can navigate to staff/system/expression, rehearsal marks, lyrics, and sticking that are placed on rests. For fingerings, it should be able to reach all notes in a chord, even with multiple voices. It does skip grace notes, but from what I saw it seems to be a wider issue of grace notes not being their own notes, which means it can't have sticking/lyrics etc attached to them. Navigating by alt-arrows (i.e. nextElement/prevElement) also skips them quite randomly, so I've opted to not deal with them here.

kennysiucho avatar Oct 18 '24 19:10 kennysiucho

@Nekotizimo great work on this, thank you!

The only thing I noticed is that the sound flag icon that shows up when you input staff text looks disabled for some reason. I don't know if it's caused by this PR or not, though in the latest nightly it's not an issue. @cbjeukendrup should we merge this anyway and log that as a separate issue?

image image

Expected:

image image

Also FYI @Nekotizimo, would you be interested in continuing work on text navigation enhancements? I have some ideas that I need to run by the team first, but if we move forward with them perhaps you would like to pick up that project.

avvvvve avatar Oct 21 '24 13:10 avvvvve

The sound flag issue is there on the latest master branch, but not the latest 4.4.3, so we should probably create a separate issue for it.

I'm happy to continue working on text navigation stuff. I'm only working on MuseScore in my free time though, so the pace of work will probably be similar to what I've done for this PR. Let me know if that's ok.

Other than that, it seems like @cbjeukendrup has proposed to refactor TextBase and text editing. I'm wondering if working on text navigation at this moment might interfere with that?

kennysiucho avatar Oct 21 '24 21:10 kennysiucho

Weird, I don't see it on the master branch. I'll approve as is and log it separately if I still see it in the nightlies once merged. Thanks!

And no problem about your working pace, we've got plenty of time until the next release.

avvvvve avatar Oct 21 '24 21:10 avvvvve

Re-tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved https://github.com/musescore/MuseScore/issues/22440 FIXED https://github.com/musescore/MuseScore/issues/24295 FIXED https://github.com/musescore/MuseScore/issues/13148 FIXED

zacjansheski avatar Oct 28 '24 16:10 zacjansheski