Skip to content

Bar numbers revamp #28662

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

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

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Jul 2, 2025

Complete rewrite of the "Measure numbers" style page including few relevant new options:

  • Bar numbers can select different text style types
  • When not set to "All staves", bar numbers now follow System Object staves and their visibility can be controlled via the Layout panel.

image

@mike-spa mike-spa requested a review from its-not-nice July 2, 2025 15:35
@mike-spa mike-spa force-pushed the barNumbersRevamp branch 2 times, most recently from 93c612f to a754af1 Compare July 4, 2025 16:05
@mike-spa mike-spa force-pushed the barNumbersRevamp branch 2 times, most recently from f6379fc to 10b42ca Compare July 7, 2025 14:16
@mike-spa mike-spa force-pushed the barNumbersRevamp branch from 10b42ca to 3e46671 Compare July 8, 2025 12:21
@mike-spa mike-spa force-pushed the barNumbersRevamp branch from 3e46671 to 4db8b3b Compare July 8, 2025 14:39
connect(item, &StyleItem::valueChanged, this, [this]() { emit measureNumberPosAboveChanged(); });
}

return item;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbjeukendrup Is this ok? I couldn't think of a better way of doing this (connecting the changing signal of one style to the UI of another). In this specific case, the "Offset" in this style page should match the offset of the chosen text style

Copy link
Member

Choose a reason for hiding this comment

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

Instead of overriding buildStyleItem, you could perhaps do the following in the MeasureNumbersPageModel constructor:

    connect(measureNumberTextStyle(), &StyleItem::valueChanged, this, [this]() { emit measureNumberPosAboveChanged(); });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing the connect in the constructor causes an assertion failure in ioccontext.cpp line 41 😭

Copy link
Member

Choose a reason for hiding this comment

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

Hm. Then it would need to be done in a load or classBegin method...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave it as it is, for now, this PR needs to be merged. I'll come back to it later if needed

connect(item, &StyleItem::valueChanged, this, [this]() { emit measureNumberPosAboveChanged(); });
}

return item;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of overriding buildStyleItem, you could perhaps do the following in the MeasureNumbersPageModel constructor:

    connect(measureNumberTextStyle(), &StyleItem::valueChanged, this, [this]() { emit measureNumberPosAboveChanged(); });

@mike-spa mike-spa force-pushed the barNumbersRevamp branch from eb5cbaa to 4b4e855 Compare July 9, 2025 12:22
@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Jul 9, 2025
@its-not-nice its-not-nice requested a review from miiizen July 9, 2025 13:46

bool isTopStave = score()->staves().front() == this;
bool isSystemObjectStaff = muse::contains(score()->systemObjectStaves(), const_cast<Staff*>(this));
return isTopStave && m_showMeasureNumbers != AutoOnOff::OFF || isSystemObjectStaff && m_showMeasureNumbers == AutoOnOff::ON;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return isTopStave && m_showMeasureNumbers != AutoOnOff::OFF || isSystemObjectStaff && m_showMeasureNumbers == AutoOnOff::ON;
return (isTopStave && m_showMeasureNumbers != AutoOnOff::OFF) || (isSystemObjectStaff && m_showMeasureNumbers == AutoOnOff::ON);

@@ -117,6 +117,7 @@ static const QStringList ALL_TEXT_STYLE_SUBPAGE_CODES {
"copyright",
"page-number",
"measure-number",
"measure-number-alternate",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be handled in EditStyle::subPageCodeForElement

RowLayout {
spacing: 4

StyledDropdown {
Copy link
Contributor

Choose a reason for hiding this comment

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

StyledDropdown is broken and opens behind the style dialog. This is due to (our favourite!) Qt widgets and QML components not playing nicely together. I think the root cause is that StyledDropdown doesn't implement the QML popup type. I battled this for a long time when making the chord symbols page and eventually had to give up and create the ComboBoxDropdown element to use in the style dialog.
Screenshot 2025-07-11 at 08 56 36

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

Successfully merging this pull request may close these issues.

4 participants