-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Introducing new options for ties, and a general overhaul of tie layout #19127
Conversation
21af4ee
to
14076b4
Compare
ab0d6bc
to
dfa016c
Compare
dfa016c
to
1d2f134
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a first round of comments, mainly about the 'supporting code'. Now I'm going to look into the new layout code.
@@ -312,6 +308,10 @@ void TieSegment::computeBezier(PointF shoulderOffset) | |||
|
|||
void TieSegment::adjustY(const PointF& p1, const PointF& p2) | |||
{ | |||
/***************************************** | |||
* DEPRECATED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right that these deprecated methods are only preserved for use in the stable
layout code? In that case, it might be desirable to remove them here and move them as static functions to stable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's quite a time consuming refactoring step, and I don't think it's worth doing it now for a method that is going to be deleted anyway. There were a few things (and there still is, to less extent) in Tie layout that were not moved to the dedicated layout class. Once we replace dev layout into stable, I've made a note to myself to go back and delete these methods that aren't used anymore.
src/engraving/types/typesconv.h
Outdated
static String toXml(TiePlacement interval); | ||
static TiePlacement fromXml(const String& str, TiePlacement def); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like AsciiStringView
is usually used for this kind of thing instead of String
src/inspector/types/directiontypes.h
Outdated
@@ -42,8 +42,15 @@ class DirectionTypes | |||
HORIZONTAL_RIGHT | |||
}; | |||
|
|||
enum TiePlacement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say DirectionTypes
is not the best place for this. I'd create a new SlurTieTypes
class/file instead.
@@ -70,4 +70,19 @@ Column { | |||
{ text: qsTrc("inspector", "Below"), value: DirectionTypes.VERTICAL_DOWN } | |||
] | |||
} | |||
|
|||
PlacementSection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not much benefit in using PlacementSection
here, since all its properties are overridden; better use FlatRadioButtonGroupPropertyView
directly.
1d2f134
to
40191d2
Compare
Note* startN = startNote(); | ||
Note* endN = endNote(); | ||
|
||
return (startN && startN->chord()->staffMove() != 0) || (endN && endN->chord()->staffMove() != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the desired formula, or should it be
return startN && endN && startN->chord()->vStaffIdx() != endN->chord()->vStaffIdx();
or, like Slur::isCrossStaff
return startCR() && endCR()
&& (startCR()->staffMove() != 0 || endCR()->staffMove() != 0
|| startCR()->vStaffIdx() != endCR()->vStaffIdx());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's correct as it is (we consider it "cross" even if both endpoints are moved to the same staff). Actually, I think the last OR condition in Slur::isCrossStaff is redundant (it can't be true if at least one of the first two isn't) but I'll verify that another time
Let's not worry about those unit tests; I'll just review #18722 asap so that we can merge that first and then we simply rebase this one. (I guess that was your plan already :) ) |
4eeb87b
to
8e619fb
Compare
@cbjeukendrup besides my comment replies, all done, thanks! |
8e619fb
to
16499d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a couple more things, but decided to fix them my self in order not to bother you again :)
src/inspector/view/qml/MuseScore/Inspector/notation/lines/SlurAndTieSettings.qml
Outdated
Show resolved
Hide resolved
16499d9
to
a62d6f9
Compare
Hm, it looks like the tests still need one more update. @mike-spa could you take care of that? (you don't need to wait for a new approval after that, since it only seems to involve test reference files) |
Move out piece of code to separate function Introduce TiePlacement property and property type correction Introduce inspector section for tie placement Created placeholder style widget for tie inside/outside Update calculation of inside/outside according to new settings Remove responsibility for setting pos out of adjustY Rename SlurPos to SlurTiePos Move computation of start and end system out of tie pos correction Temporarily deactivate adjustX and adjustY Rationalize calculation of default start/end points Force horizontal correction Re-implementation of adjustX correction Rationalize shoulder height formula and increase default curvature Decrease thickness for short ties Extract thickness calculation to dedicated method initial reimplementation of adjustY Implementing arc vs staffLine avoidance correction Better management of short ties more Don't do adjustments if autoplace is off Correct dragging behaviour cleanup Fix glitches when when dragging ties correction improvements Adjust for ledger lines corrected values Fix tie-to-tie collisions Tweak curve correction correction correction margins above and below correction move some constants to style correction Final corrections
Update template files
a62d6f9
to
06ca4f2
Compare
Code review corrections - 2 Code review corrections - 3 Code review corrections - 4 Code review corrections - 5
06ca4f2
to
3dfae78
Compare
Resolves: #18685
This PR implements the feature requested in #18685. In order to do that, a deep overhaul of the tie layout code became necessary, as the existing code was too complex and inadequate to achieve this.
Here's a (probably incomplete) summary of the changes.