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

[MU4 Issue] Tablature: Ctrl + Up/Down does not move fretmark to adjacent string (note input mode) #16487

Closed
rgreen5 opened this issue Feb 22, 2023 · 9 comments · Fixed by #18424
Assignees

Comments

@rgreen5
Copy link

rgreen5 commented Feb 22, 2023

Describe the bug

A number of bugs have been flagged up in which Ctrl + Up/Down no longer works to move a fretmark to an adjacent string (preserving pitch) in note input mode. See #14218, #15344.

However this bug is applicable in all situations – not just where there are linked staves or an ottava or capo is operative.

To Reproduce

  1. Create a blank tablature staff.
  2. Enter note input mode.
  3. Add a fretmark at string 2, fret 5 (musical note E).
  4. Use Ctrl + Up/Down to move the fretmark to an adjacent string.

RESULT: The cursor moves but the fretmark doesn't!

Expected behavior

The above operation should move the fretmark to an adjacent string.

Platform information

OS: Linux Mint 20.1, Arch.: x86_64, MuseScore version (64-bit): 4.0.2-230530504, revision: github-musescore-musescore-fa46d7f

@oktophonie
Copy link
Contributor

Works fine for me. Better than ever, since #15730.

@cbjeukendrup
Copy link
Contributor

@rgreen5 Is this still actual?

@cbjeukendrup cbjeukendrup added the needs info More information is required before action can be taken label Jul 4, 2023
@rgreen5
Copy link
Author

rgreen5 commented Jul 5, 2023

OS: Linux Mint 20.1, Arch.: x86_64, MuseScore version (64-bit): 4.1.0-231860501, revision: github-musescore-musescore-1654979

The bug can still be reproduced in this nightly in note input mode using the instructions in the OP (it works OK in normal mode). It worked corrctly in MS 3.6.2 using the same instructions.

@MarcSabatella
Copy link
Contributor

I confirm it's still an issue in 4.1.0. The PR #15730 was for a different bug, one only affecting the highest string (and only visible in normal mode). It remains the case that Ctrl+Up/Down are ineffective in note input mode for tab - they simply move the cursor.

@cbjeukendrup
Copy link
Contributor

Aha, now I understand what this is about. I thought that this was expected behaviour in Note Input mode, but of course the Up/Down arrows already move the cursor without Ctrl, so if would make sense for Ctrl+Up/Down to do something different (namely moving the selected note).

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Jul 5, 2023

Right. I looked at the code briefly to see if it was just a simple matter of something not being hooked up properly as was the case for a number of other commands, but offhand I don't see the problem. Ctrl+Up activates the pitch-up-octave command and I see that on the console, and the handler looks like it deals with the case of note input + tab. But presumably it's something simple that would be obvious on stepping through.

@MarcSabatella
Copy link
Contributor

OK, I see now why this is the case, but I'm not sure of the right fix. Tracing the control flow, I see we eventually arrive in NotationInteraction::moveStringSelection(), which appears to be the function for moving the cursor, not for moving notes. Probably it should be doing both.

I'll make a trial PR with what might be the answer, but no guarantees.

@MarcSabatella
Copy link
Contributor

My PR fixes the issue, but the behavior is slightly different than MU3 in that MU3 leaves the note cursor on the original string whereas mine moves the cursor along with the note. I'm not convinced the MU3 behavior is better, but if we want this, it would be simple enough (by putting the code to move the cursor into an "else" clause, so it only happens when not using Ctrl).

@rgreen5 what do you think?

@rgreen5
Copy link
Author

rgreen5 commented Jul 6, 2023

Marc's fix looks fine to me.

@zacjansheski zacjansheski removed the needs info More information is required before action can be taken label Jul 6, 2023
@zacjansheski zacjansheski self-assigned this Jul 6, 2023
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 a pull request may close this issue.

5 participants