-
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
fix(time signatures): adding a time signature does not deselect current measure(s) #24891
base: master
Are you sure you want to change the base?
fix(time signatures): adding a time signature does not deselect current measure(s) #24891
Conversation
Isn't this a duplicate of #24892 (or vice versa)? |
Yes, this is the correct one because it merges into master |
src/engraving/dom/edit.cpp
Outdated
} | ||
} | ||
std::pair<staff_idx_t, staff_idx_t> staffIdxRange = getStaffIdxRange(score); | ||
for (staff_idx_t si = staffIdxRange.first; si < staffIdxRange.second; ++si) { | ||
TimeSig* nsig = toTimeSig(seg->element(si * VOICES)); | ||
if (!nsig) { | ||
TimeSig* newSig = toTimeSig(seg->element(si * VOICES)); |
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.
better newTimeSig
to differentiate from key signatures
src/engraving/dom/edit.cpp
Outdated
Fraction ns = ts->sig(); | ||
Fraction tick = fm->tick(); | ||
TimeSig* lts = staff(staffIdx)->timeSig(tick); | ||
Fraction newSigFraction = timeSig->sig(); |
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.
Better newTimeSigFraction
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.
also loose the extra space, unless using them for alignment
This is a draft btw, you don't have to review now! I thought the convention was to mark as 'ready for review' when I am ready for review @Jojo-Schmitz |
mf->undoChangeProperty(Pid::TIMESIG_NOMINAL, newSigFraction); | ||
Measure* currentMeasure = mf->nextMeasure(); | ||
Segment* segment = currentMeasure->findSegment(SegmentType::TimeSig, currentMeasure->tick()); | ||
mf = segment ? 0 : mf->nextMeasure(); |
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.
@Jojo-Schmitz Any idea what this 'mf' is supposed to refer to?
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.
To do with steming from master score I guess?
@@ -4651,6 +4651,7 @@ void NotationInteraction::addTimeSignature(Measure* measure, staff_idx_t staffIn | |||
startEdit(); | |||
score()->cmdAddTimeSig(measure, staffIndex, timeSignature, true); | |||
apply(); | |||
doSelect({ measure }, SelectType::SINGLE, staffIndex); |
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.
@Jojo-Schmitz This is my first approach at a fix.
a) Is it sensible?
b) How do I start to approach testing? The tests don't all pass locally on my machine, but it seems they do fine in the CI.
Given that I'm not likely to be doing much more work on this after this fix, am I ok to test just by shoving commits at the CI and then we can squash them all afterwards?
Otherwise I will need (likely several hours/days worth of) support from existing contributors to debug my local test setup and learn a bunch of new tooling
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.
Sirry, no idea, beyond my pay grade...
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.
@Jojo-Schmitz So you don't do any testing or what?
How am I supposed to tell whether my code has actually done anything?
Those merge commits should not be there, please try a |
15480cd
to
e21a5ad
Compare
Done |
Resolves: #9584