Skip to content

Conversation

@miiizen
Copy link
Contributor

@miiizen miiizen commented Nov 13, 2025

Refinements to inspector controls and text style dialog controls.
Screenshot 2025-11-13 at 15 38 33

Also, aligns the left/center/right of the text box to the left/center/right of the note it's being aligned to.

Screen.Recording.2025-11-13.at.15.37.54.mov

@miiizen miiizen requested review from a user and avvvvve November 13, 2025 15:39
@avvvvve
Copy link

avvvvve commented Nov 13, 2025

Looks nice to me! A couple tiny tiny things:

The tooltip copy in Style for center-aligning the text box says "Horizontally center text box on reference point" but "on" should be "to" to match properties and the other tooltip copy.
image

Also, could you tweak the vertical spacing between the rows of buttons in both spots plz? Horizontal spacing can stay as-is.

  • Properties: 8px
  • Style: 4px
image

@avvvvve
Copy link

avvvvve commented Nov 18, 2025

Nice! All good to me.

@ghost ghost added the vtests This PR produces approved changes to vtest results label Nov 19, 2025
@ghost ghost requested a review from mike-spa November 19, 2025 08:07
return false;
}

bool useBl = isMarker() || isJump() || isRehearsalMark() || isMeasureNumber() || isPlayCountText();
Copy link
Contributor

Choose a reason for hiding this comment

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

What does Bl stand for? The naming is a bit obscure and in general this reads a bit wonky with all the negations, although there may not be clearer way to express the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Barline - took me a second to work it out too! I've made this more clear

|| parent()->isSpannerSegment();

return !relativeToBarline && !harmonyRelativeToBarline && !textRelativeToBarline && !noPositionControl;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Reads a bit better now, but may I suggest rewriting this in some form like:

switch (type()) {
case ElementType::MARKER:
case ElementType::JUMP:
case ElementType::MEASURE_NUMBER:
case ElementType::PLAY_COUNT_TEXT:
    return false;
case ElementType::HARMONY:
    return !parent()->isFretDiagram();
case ElementType::SOMETHING_ELSE:
    return somethingElse;
default:
    return basicConditions;
}

Cause it still reads weird to be definining a "relative to noteheade" property as "not relative to barline". Besides, those are a lot of boolean checks, a switch should be a bit more efficient (though I'm sure it hardly matters here).

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, a purist would say that this should be a virtual method and specific TextBase classes should implement their own versions. Either way is fine for me, I'll leave it up to you :)

Change tooltips in inspector

Refine align and position buttons in edit style

Fix button spacing

Add vtest

Migrate old scores

Refactor `positionRelativeToNoteheadRest`
@miiizen miiizen merged commit 11d4e5c into musescore:master Dec 3, 2025
12 of 14 checks passed
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.

3 participants