Skip to content

French style beams option #21126

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

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Conversation

XiaoMigros
Copy link
Contributor

@XiaoMigros XiaoMigros commented Jan 21, 2024

This PR adds a style option to draw stems up to the first beam rather than the top beam, where suited

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

@its-not-nice
Copy link
Contributor

Just a couple of problem situations:
image

Something odd with tuplets (though, strangely, not with non-tuplets):
image
image

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 is a lovely addition.
From my POV, the only thing that probably needs rethinking is the UX/UI. My preference would be for a more visual toggle (2 buttons, à-la the "Beam distance" buttons, or even the "Ties" buttons), instead of the current checkbox.

Screenshot 2024-01-25 at 11 36 02 am

This will require the briefest bit of design work. @avvvvve do you have capacity at the moment to tackle this?

@XiaoMigros
Copy link
Contributor Author

I can take care of it if desired, except for the icons and their font.

@avvvvve
Copy link

avvvvve commented Jan 26, 2024

@XiaoMigros Thanks! I know @oktophonie already passed off the Figma link, but I'm posting a design screenshot here too for reference.

image

@XiaoMigros XiaoMigros force-pushed the french-style-beams branch 11 times, most recently from dbcf301 to ab4c1c9 Compare January 30, 2024 18:15
@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Jan 31, 2024
@XiaoMigros XiaoMigros force-pushed the french-style-beams branch 6 times, most recently from 0cd6a7f to c943cc8 Compare January 31, 2024 15:09
@XiaoMigros XiaoMigros force-pushed the french-style-beams branch 3 times, most recently from 60d35a4 to d03ebb1 Compare February 9, 2024 19:45
@igorkorsukov igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08
@bkunda bkunda self-requested a review February 16, 2024 13:02
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.

I love this.
Thanks @XiaoMigros. Good to go from my end!

background: Rectangle {
y: root.topPadding - root.bottomPadding
width: parent.width
height: parent.height - root.topPadding + root.bottomPadding
Copy link
Member

Choose a reason for hiding this comment

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

This line too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed here to calculate the positon of the Rectangle relative to the text, bottomPadding and topPadding include label height.

@XiaoMigros XiaoMigros force-pushed the french-style-beams branch 8 times, most recently from ccb29fa to bf6e010 Compare February 20, 2024 00:18
background: Rectangle {
y: root.topPadding - root.bottomPadding
width: parent.width
height: parent.height - root.topPadding + root.bottomPadding
Copy link
Member

Choose a reason for hiding this comment

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

But I really don't get why it is calculated like this. Don't you mean

Suggested change
height: parent.height - root.topPadding + root.bottomPadding
height: parent.height - (root.topPadding + root.bottomPadding)

?

In that case you can also use

Suggested change
height: parent.height - root.topPadding + root.bottomPadding
height: root.availableHeight

, right?

Copy link
Member

Choose a reason for hiding this comment

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

Okay I see now that you got it from https://doc.qt.io/qt-5/qtquickcontrols2-customize.html#customizing-groupbox. So either that tutorial is weird or I just don't get it. Anyway, if it works, it works.

@cbjeukendrup cbjeukendrup merged commit a81416d into musescore:master Feb 21, 2024
@XiaoMigros XiaoMigros deleted the french-style-beams branch February 21, 2024 05:49
@Jojo-Schmitz
Copy link
Contributor

Is this going into 4.3.0 too?

@cbjeukendrup
Copy link
Member

No, it's currently scheduled for 4.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engraving vtests This PR produces approved changes to vtest results
Projects
Development

Successfully merging this pull request may close these issues.

7 participants