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

Fix cross system anchors issues #23343

Merged
merged 14 commits into from
Jul 5, 2024

Conversation

mike-spa
Copy link
Contributor

Resolves: #23327

This PR resolves points 1-3 of the related issue. Namely:

  • Lets items snap to each other also across system breaks (contrary to what happens now, whereby system breaks also break any snapping chain). This lets a dynamic keep its preceding hairpin attached if the hairpin is in the previous or next system.
  • Lets hairpins also move around their snapped items when shift+arrowing (contrary to what happens now, where only dynamics can do that).
  • Makes sure that shift+arrow editing works on a split-system-hairpins.

This PR doesn't attempt to solve point 4, i.e. dragging items to a different system, because that's something that's always been a jankfest in Musescore so it needs separate work. But also because dragging an item like that is a criminal offense sanctioned by every country in the United Nations.

Copy link

@avvvvve avvvvve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The things this fixed look great, but it looks like this introduced a new issue. Now, arrow key presses with no modifier no longer nudge the full hairpin around, they instead are doing the same thing as when you press shift and hold arrow keys. Also, after pressing an arrow key, the pink anchor lines and striped background layer stay lit up.

Additionally, Cmd + Arrow key has the same behavior of arrow keys on their own as described above.

We've also lost the ability to lengthen a hairpin by an entire measure (previously Shift + arrow left/right). Can we reintroduce that with Cmd + Shift as the modifier?

Screen.Recording.2024-06-24.at.4.59.19.PM.mov

@mike-spa mike-spa force-pushed the fixCrossSystemAnchorsIssues branch from 7b2edfb to b11725a Compare July 1, 2024 10:32
@mike-spa
Copy link
Contributor Author

mike-spa commented Jul 1, 2024

@avvvvve done!

@avvvvve
Copy link

avvvvve commented Jul 3, 2024

First off, apologies for how long this is but I've been doing a lot of fiddling with this PR the past couple days! Issues 1 and 2 are probably addressable in this PR, but the rest can be logged as separate issues/debated philosophically. @mike-spa @oktophonie @bkunda

Issue 1 (probably should fix in this PR)

Hairpins become uncentered after undo.

Screenshot 2024-07-01 at 11 37 38 AM
  1. Create a two-staff instrument and drag in a staff spacer to make the gap between staves obvious.
  2. Add a hairpin to a two-staff instrument (so that it’s automatically centered)
  3. Move the hairpin with an arrow key
  4. Press undo
  5. The hairpin becomes uncentered between the staves, but its centering property is still set to “Auto”. There’s no way to re-center it without deleting it and adding a new one.

Issue 2 (simple)

Already logged #23435

When double-clicking onto a dynamic to edit its text field, the entry cursor line is too tall now (first screenshot is this PR, second is 4.3)
Screenshot 2024-07-01 at 11 37 38 AM Screenshot 2024-07-01 at 11 37 56 AM

Issue 3 (probably not solvable now)

Interacting with the "Snap to next/previous" toggles in properties makes it impossible to move hairpins with arrow keys until they are selected again since focus has now moved to the properties panel.

Issue 4 (more of a philosophical manifesto on how dynamics should move around)

How should horizontally moving a hairpin with dynamics snapped to its end(s) behave when snapping is turned on or off? Specifically talking about moving the entire hairpin without using the shift modifier.

Current behavior while moving with arrow keys

(Expand sections below for gif examples)

Dynamic BEFORE hairpin, “Snap to previous” ON, move hairpin with left arrow: Left dynamic only: hairpin and dynamic keep vertical position but hairpin moves over the dynamic (colliding with it)

Hairpin on both sides: same as above, but the right side of the hairpin jumps to the right at a certain point

Dynamic BEFORE hairpin, "Snap to previous" OFF, move hairpin with left arrow: Left dynamic only: dynamic moves out of the way of the hairpin

Hairpin on both sides: same as above, but the right side of the hairpin jumps to the right at a certain point

Dynamic AFTER hairpin, "Snap to previous" ON or OFF (it makes no difference), move hairpin with right arrow: Right dynamic only: right edge of hairpin stops moving where the dynamic is, and it starts squishing horizontally

Hairpin on both sides: same as above, but the left edge moves away from the first dynamic (compare to when moving the hairpin left, the right edge staying attached to the ending dynamic)

Deleting a "squished" dynamic after it's been moved When you've moved a hairpin that's been "squished" by an end dynamic, deleting the dynamic will restore the hairpin to its original length before it was moved.

Current behavior while moving with mouse

In 4.3, if you click and drag an entire hairpin (i.e. using the middle handle), it would re-anchor to whatever beats it was closest to. You could prevent re-anchoring while dragging left/right by holding Cmd (Mac) after beginning the drag. Now in the PR, there’s no way to re-anchor while dragging, unless you use the handles. That could be desirable, but we should note the change on release.

Also in the PR, try dragging a hairpin that has a dynamic at the end of it to the right of the dynamic. When released, it will get really small (regardless of the state of the snapping toggles). It should just stay the same size when dragged around.

Suggested behavior for moving hairpins horizontally

There are a couple ways to approach this, with more detailed explanations below:

  1. Horizontally moving a hairpin should not affect the position of dynamics it is snapped to.
  2. Horizontally moving a hairpin should also move the dynamics that are snapped to it along with it.

Suggested behavior 1:

  • If both "Snap to previous" and "Snap to next" are turned OFF, moving the hairpin should ignore all surrounding dynamics and move anywhere. Its length should be retained.
  • If "Snap to previous" is ON and "Snap to next" is OFF, left arrow should nudge the entire hairpin and the dynamic to its left together, leaving the dynamic on the right untouched.
  • If "Snap to previous" is OFF and "Snap to next" is ON, right arrow should nudge the entire hairpin and the dynamic to its right together, leaving the dynamic on the left untouched.
  • If both "Snap" options are turned on, using the left/ right arrows should move the hairpin AND both its beginning and end dynamic together as one unit (this is close to how a hairpin with dynamic beginning/end text currently works).

Suggested behavior 2:

  • If both "Snap to previous" and "Snap to next" are turned OFF, moving the hairpin should ignore all surrounding dynamics and move anywhere. Its length should be retained. (same as above)
  • If "Snap to previous" is ON and "Snap to next" is OFF, left arrow should move the right edge only and shrink the hairpin leftward.
  • If "Snap to previous" is OFF and "Snap to next" is ON, right arrow should move the left edge only and shrink the hairpin rightward.
  • If both "Snap" options are turned on, using the left/right arrows won't move the dynamic at all (this is a little weird).

A note on the "Dynamics + hairpin" object

There are a few pre-4.4 ways of appending or prepending a dynamic to a hairpin, all which utilize the text tab of Hairpin properties.

dynamic-plus-hairpin
  • Add a hairpin, then with it selected add a dynamic from palettes. This adds “End text” to the hairpin, which uses some text syntax. Here, the dynamic and hairpin are a single element: when the hairpin is moved, the dynamic is moved.

    • Users can also just type in a dynamic directly into the text properties if they know the syntax.
  • The other way is to add the “Dynamic + hairpin” palette element. This palette element is still available in 4.4, but it is still utilizing this “old” way of adding a hairpin/dynamic combo. Perhaps instead, it should add separate hairpin and dynamic elements so that they can be unsnapped and snapped using the new properties.

Also, the palette only accounts for adding a dynamic followed by a hairpin. We could also include palette items for 1. hairpin followed by dynamic and 2. dynamic + hairpin + dynamic. OR, we could just get rid of these elements altogether since the GSoC project will make adding dynamics and hairpins in-score significantly easier.

@cbjeukendrup
Copy link
Contributor

"Issue 2" is also known as #23435, and I really hope it is not yet another Qt 6 issue.

@mike-spa mike-spa force-pushed the fixCrossSystemAnchorsIssues branch from b11725a to cfcf1aa Compare July 3, 2024 16:49
@avvvvve
Copy link

avvvvve commented Jul 4, 2024

I crashed this one by doing this:

  1. Enter a hairpin in measure one (or at the start of a system)
  2. Select the hairpin
  3. Drag the hairpin left
  4. Crash

It also crashes if you put a hairpin at the end of a system and try to drag it to the right.

@mike-spa mike-spa force-pushed the fixCrossSystemAnchorsIssues branch from cfcf1aa to 5ff29ac Compare July 4, 2024 14:30
@mike-spa
Copy link
Contributor Author

mike-spa commented Jul 4, 2024

Update.

  1. ✅ Fixed. This, by the way, is yet another manifestation of the nuclear disaster of offsets. There is no good fix, so I've done a workaround that I just hope holds long enough.
  2. ❌ Not to be fixed here (and also @cbjeukendrup I suspect that this may be a mac-specific issue, cause the cursor looks normal-sized on my machine).
  3. ❌ Not solvable
  4. ✅ Done. I had intentionally disabled the re-anchoring of hairpins when dragged by the middle grip (i.e. moved as a whole) because it was causing problems. I've now reintroduced it and refined it in a way that also takes care of the elements snapped to it (see video). The re-anchoring is not smooth, i.e. happens in jumps from one anchor point to the next, but that's not something that I can work on now (we've really hit the limit of how much time I can dedicate to refining this feature). See:
20240704_163239.mp4
  1. 💥 The hairpin+dynamic element in the palette has been happily nuked.
  2. Fixed the crashes

@RomanPudashkin RomanPudashkin merged commit a1e283a into musescore:master Jul 5, 2024
10 of 11 checks passed
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 this pull request may close these issues.

Anchors and snapping improvements
5 participants