Skip to content

Conversation

@XiaoMigros
Copy link
Contributor

Resolves: #18769

This PR introduces separate style settings for slurs and ties, with accompanying UI changes to the style dialog.

  • 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)

@XiaoMigros XiaoMigros changed the title Independent thickness settings for slurs and ties Independent style settings for slurs and ties Dec 24, 2023
@cbjeukendrup
Copy link
Member

Looks like one MusicXML unit test still desires to be updated...

@XiaoMigros
Copy link
Contributor Author

The test seems to fail on every mxl test file, but I don't see any issues with my code or the test's code. Do the test files need to be updated?

@cbjeukendrup
Copy link
Member

Yes, in this case the test file needs to be updated. (Note that the diff shown in the test output may feel like it's the wrong way around, so the "+" lines are what's currently in the reference file and the "-" lines are what it needs to become.)

Copy link

@bkunda bkunda left a comment

Choose a reason for hiding this comment

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

This generally looks fine to me.
If I were to nitpick about a tiny UI issue, there should be a gap between the bottom of the "Slurs & Ties" section and the top of the "Ties" section.

Screenshot 2023-12-28 at 12 35 10 pm

The "System" page has what looks to be roughly 12px of padding here (perhaps this could be used as a reference).

styles-system

Otherwise it's great! Thanks @XiaoMigros!

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 28, 2023
@XiaoMigros
Copy link
Contributor Author

XiaoMigros commented Dec 28, 2023

Thanks for the feedback @bkunda! The layout now matches that of other pages in the dialog.

I also changed style.cpp to automatically adopt the slur values for ties as well, when importing older scores. This means the vtests are now successful, and the changes made to the MXL test files have been reverted (@cbjeukendrup).

@XiaoMigros

This comment was marked as outdated.

@bkunda
Copy link

bkunda commented Jan 30, 2024

I've given this another test on macOS.

Only thing I noticed was that modifying settings for Slurs seemed to affect my ties as well. At the same time, modifying "Line thickness middle" for ties didn't have any affect at all. See video

@cbjeukendrup
Copy link
Member

Another rebase is needed since the recent work in the MusicXML module...

@cbjeukendrup
Copy link
Member

From the combination of Bradley's latest comment and Simon's approval after the last fix, I assume that this is ready for merging, right?

@cbjeukendrup cbjeukendrup merged commit da7775d into musescore:master Feb 2, 2024
@XiaoMigros XiaoMigros deleted the slurtie-thickness branch February 2, 2024 10:16
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 2, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 2, 2024
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 9, 2024
* Independent style settings for slurs and ties

Backport of musescore#20693

* Update translations

---------

Co-authored-by: XiaoMigros <[email protected]>
@its-not-nice its-not-nice removed the request for review from RomanPudashkin July 1, 2024 12:31
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.

Allow independent thickness settings for slurs and ties

5 participants