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

Partial ties refinements #26048

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Jan 10, 2025

This PR contains:

  • The last few fixes from Casper's review of the first PR
  • Moves TieJumpPoint related code to its own file
  • Simplify the relationship between TieJumpPoint and Note
  • Updates the minimum hanging tie length to 1.5sp
  • Fixes the following crash
    https://github.com/user-attachments/assets/a59055ff-bfe8-41c3-9649-bedaec01e235
  • Fixes ties to next notes being named "invalid"
  • Refactors the tie popup. This never needed to use the whole StyledMenu implementation and definitely didn't need a loader. I've recreated the popup in a more straightforward way.


return title;
//: %1 represents the preceding jump item eg. coda. %2 represents the measure number
return muse::mtrc("engraving", "Tie to %1 (m. %2)").arg(precedingJumpItemName(), measureStr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I overlooked that measureNo is an int of course. You could do

return muse::mtrc("engraving", "Tie to %1 (m. %2)")
       .arg(precedingJumpItemName(), String::number(measureNo));

(Alternatively,

return muse::mtrc("engraving", "Tie to %1 (m. %2)")
       .arg(precedingJumpItemName()).arg(measureStr);

but that is less performant.)

@@ -33,7 +33,7 @@ StyledPopupView {
property alias navigationOrderStart: capoSettingsNavPanel.order
readonly property alias navigationOrderEnd: capoSettingsNavPanel.order

property QtObject model: capoModel
readonly property alias model: capoModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this property at all, now that the model.canOpen call no longer happens in QML but in C++?

#pragma once
#include "global/types/string.h"

using muse::String;
Copy link
Contributor

Choose a reason for hiding this comment

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

using … in header files is not really preferred because it may cause conflicts in other cpp files that include this header file. In this case I'd just write muse::String.

import Muse.UiComponents 1.0
import MuseScore.NotationScene 1.0

ListItemBlank {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the accessibility/navigation stuff from StyledMenuItem would need to be ported over as well

Layout.fillWidth: true
horizontalAlignment: Text.AlignLeft

text: modelData.title
Copy link
Contributor

Choose a reason for hiding this comment

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

With https://mu--se.slack.com/archives/C013WRMQ5M5/p1734490004476809?thread_ts=1734489990.108119&cid=C013WRMQ5M5 in mind, it's perhaps better to make modelData a required property of root, rather than using it via the "context".

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.

2 participants