GSoC '24 - Dynamics Popup - Part 3

[ketgg]

This is the third part of the main pull request: #23038, which adds functionality to add dynamic using shortcut, opening of popup when hairpin ends on a note or rest and the automatic switching of hairpin type.

PR for part 1: #24125
PR for part 2: #24147

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• 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)

[avvvvve]

Apologies for the time it's taken to start reviewing parts 2-4, but here's some feedback:

Issue 1

After dragging a hairpin out from a dynamic, I don't see the popup appear. I'm assuming it's supposed to in this PR based on the description?

Screen.Recording.2024-10-22.at.5.34.16.PM.mov

Issue 2 (crash)

If a hairpin has a dynamic at either end but NOT both ends, and I click the dynamic and change it via the popup, it crashes.

1. Add a dynamic followed by a hairpin via any method (shortcut, popup, dragging, palettes - doesn't matter)
2. Click the dynamic
3. Click any dynamic in the popup
4. Crash

Screen.Recording.2024-10-22.at.5.38.24.PM.mov

The automatic switching of hairpin direction looks good!

[cbjeukendrup]

Note: some code in this PR was made redundant by #25252 (update: solved)

[cbjeukendrup]

@avvvvve Ready for retesting!
I feel stupid for not realising this way earlier, but of course Shift+X has exactly the same problem as just Shift. So I opted for Alt+X, at least as a proof of concept.

[bkunda]

It's great @cbjeukendrup! Thank you for all your work on this.

Just a couple of things, which I show in this video (excuse the slightly exploratory ramblings):

1. The "end" dynamic after a hairpin is appearing only when shift is held. It'd be nice if it just appeared.
2. Holding down Alt+X is causing the flip command to run in an indefinite loop. It'd be better if it just flipped once (meaning the user has to release their keys and re-press them to re-flip the object).

By the by, I thought we had just opted to use Alt, not Alt+ X. Perhaps I missed something in the comms about this though? Is the addition of the X key really necessary?

GMT20250212-113620_Clip_Bradley.Kunda.s.Clip.02_12_2025.mp4

[avvvvve]

Strangely, both of the following are working as expected for me in this PR:

• Cmd + D shortcut adds dynamics
• The dynamic entry box appears right after dragging out a hairpin with or without holding down Shift

---

Re: hairpin flipping

@bkunda See discussion here and in the subsequent three comments about why we went with a command to swap the direction rather than a modifier held while dragging. The big advantage of this is you can swap the direction of a hairpin after it's been created, not only while you're resizing it, plus we avoided conflicting behaviors when Shift is held.

Casper thus added a new shortcut I suggested, Flip horizontally, that is a sister-shortcut to the existing Flip direction shortcut (X). They need to be separate because Flip direction can still be used to flip objects (including hairpins) above or below the staff (and it also flips stem direction on notes).

• Aside: We also have Mirror notehead (Shift + X), which I previously posited we could merge with Flip horizontally, but that would now require changing its default shortcut from Shift + X to Alt + X. People might not be happy with that change, so we should probably leave it as-is for now.

---

Suggestions

I think we should rename Flip direction now that there are different shortcuts for flipping in different directions. We could do:

• Flip vertically
  • Pro: concise
  • Con: it could sound like this flips an object upside down, which it does not do
• Flip stem direction / flip position (my choice ⭐️)
  • Pro: tells it like it is, and we have precedent for listing multiple different actions done by the same shortcut
    
  • Con: wordier

It could also be nice to add an icon to the Flip horizontally shortcut to to indicate what it will flip (since I assume it won't flip anything but hairpins at the moment). Janky mockup:

[avvvvve]

Another few things (point 1 is one of @bkunda's mysteries solved)

Screen.Recording.2025-02-12.at.11.11.11.AM.mov

1. (0:00–0:45) Dragging the end of a hairpin to a beat position where a note/rest exist triggers the dynamic entry popup. BUT if you drag it to, say, halfway through a quarter note, the popup does not appear because there's no note/rest there.

   • We could either keep this as is, or say that the popup should ALWAYS appear when dragging the end of a hairpin (without shift held—see point 3).
   • If we do the latter, I can anticipate some users wanting a preference to turn off automatically showing this popup, because it comes up every time you move a hairpin's end, not just while entering it for the first time.

2. (0:45–1:17) Anchor line doesn't disappear after dismissing popup/undoing hairpin drag (low priority, can file separately unless it's easy to fix)

3. (1:17–end) Holding shift while dragging the hairpin lets you move the anchor point without visually resizing the hairpin. I think we could just say that if you are dragging the hairpin with shift held, don't show the popup on release, because you're probably trying to do some fine-tuning that adding a dynamic might mess with.

[Tantacrul]

OK, just tested part 3 again for a final check before merge.

Issue 1: The shortcut to add a dynamic isn't working anymore. Click on a duration and try it. Nothing happens.

[Tantacrul]

Issue no.2:

When using Shift+Arrow Keys (MacOS) to move dynamics to different beats, they now disconnect from the hairpin, which is a pretty significant regression we'll need to fix.

Ignore me saying 'Shift+Command+Mac'. I tested again. Same behaviour with Shift+Arrow

NOTE: When dragging, they remain connected.

Video:

DYNAMIC_MOVE.mp4

[avvvvve]

Re: the shortcut, I've noticed this happen in the Nightly builds too. You can fix it by resetting the 'Add dynamic' shortcut to default in Preferences, but yeah, for some reason it's getting cleared.

Re: moving dynamics to different beats, I believe the key command is simply Shift + Arrow keys. Adding command to that doesn't change what happens (in either 4.4 or the new builds)

[Tantacrul]

Thanks @avvvvve. I've amended my comment.

[cbjeukendrup]

Regarding why the shortcut seems not to be working: in this PR, I changed the action code of the "Add dynamic" command. As a consequence, every time that you switch between a nightly build from before this PR is merged, and a build that does contain the changes from this PR, the shortcut is lost, because of how the shortcuts migration system works (it does not bind the shortcut to the new action code, because it is already bound to another action, except that that other action is the old action code so does not exist anymore). I can fix that in a separate PR, but it won't matter: end users will only ever see the new action code, so there won't be any manual reset needed. The problem only affects nightly/PR builds on computers of people who have used recent nightly/PR builds from before this PR.

Regarding Shift+arrow seemingly not working: that only works when the dynamic is at the same segment as the hairpin's start or end. That's not the case in the video: the dynamic pp is on the 4th eighth or the bar, and the hairpin starts on the 5th. Later, you are using shift+arrow on the mf dynamic, rather than on the hairpin; when doing this, master seems to show the same behaviour as in the video.

So, as far as I can see, both things are not really problems of this PR, so is it okay to merge it?

[Tantacrul]

> Regarding Shift+arrow seemingly not working: that only works when the dynamic is at the same segment as the hairpin's start or end. That's not the case in the video: the dynamic pp is on the 4th eighth or the bar, and the hairpin starts on the 5th. Later, you are using shift+arrow on the mf dynamic, rather than on the hairpin; when doing this, master seems to show the same behaviour as in the video.

Definitely, the Shift+Arrow functionality is not working as expected, since the dynamic becomes separated from the hairpin when that happens. That said, I've noticed that this is also the case with nightly build, so it's not specific to this PR.

However, one way or another, the issue is a must fix. 100%

In this instance, if I hold SHIFT and press the right arrow, the dynamic becomes separated from the hairpin

[bkunda]

Definitely, the Shift+Arrow functionality is not working as expected

It seems like we should probably merge this PR and raise a separate issue to fix the broken dynamics+hairpins connection.

@cbjeukendrup @mike-spa does this work for you both?

[cbjeukendrup]

Yes, let's do that, as the dyn/hairpin connection issue seems really something separate.

[cbjeukendrup]

See #26560