Fix #15321: Allow Removal of Spanners from Range Selections #20634
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Wouldn't changing SpannerSegment* frontSegment() { return m_segments.front(); }
const SpannerSegment* frontSegment() const { return m_segments.front(); }in spanner.h to SpannerSegment* frontSegment() { return m_segment ? m_segments.front(): nullptr; }
const SpannerSegment* frontSegment() const { return m_segment ? m_segments.front(): nullptr; }be better/safer? Likewise for |
|
|
96dc7ec
to
fc4afa7
Compare
|
I changed the selection code so that the user can only select |
fa1f8d3
to
525a11a
Compare
The fact that a single left-click selects the SpannerSegment is quite important though, cause that is what you are actually editing most of the time. Has anyone tested this? Are we sure that it is safe to make this change? |
63744ff
to
acd2b81
Compare
I see, I have now changed it so that both left-click and range selections selects Is it right to assume that we won't need to select the actual |
acd2b81
to
5ada278
Compare
The Spanner represents the "concept", whereas the SpannerSegment represents the actual graphical item that is drawn on the score (so, for instance, a slur that crosses a system break has one Slur object representing it, but is made of two SlurSegments, representing each of the two graphical arcs). When, say, the user selects and drags a point of the slur, what they are seeing and editing is the slur segment so it makes sense that that is selected. We then have internal logic which passes some of the editing operations to the Spanner when appropriate. So, this makes more sense to me now. Codewise I think it's all fine, but I personally don't feel comfortable merging this kind of changes right now, because we are very close to release and these changes look small but can be quite consequential. I'm happy to come back and merge this after the release (please shout at me in case I forget). |
|
It doesn't allow to deselect ties, slurs, hairpins that span a system break, or rather only when (de)selecting all their segments. It doesn't allow for ottavas, pedal lines, trill lines, staff or system text lines, let ring or palm mute lines at all. It does however allow to add and remove voltas (why are they not included in a range selection by default?), even if they span system breaks. |
Backport of musescore#20634, plus fixing some clang warnings
| @@ -711,14 +713,16 @@ void Selection::updateSelectedElements() | |||
| if (sp->isVolta()) { | |||
| continue; | |||
| } | |||
| if (sp->isSlur()) { | |||
| if (sp->isSlur() || sp->isHairpin()) { | |||
| // ignore if start & end elements not calculated yet | |||
| if (!sp->startElement() || !sp->endElement()) { | |||
| continue; | |||
| } | |||
| if ((sp->tick() >= stick && sp->tick() < etick) || (sp->tick2() >= stick && sp->tick2() < etick)) { | |||
There was a problem hiding this comment.
why sp->tick2() < etick here, but sp->tick2() <= etick further down? Smells like an off-by-one error to me
Backport of musescore#20634, plus fixing some clazy warnings
ottavas, pedal lines, trill lines, staff and system text lines, let ring and palm mute lines Follow up to musescore#20634
ottavas, pedal lines, trill lines, staff and system text lines, let ring and palm mute lines Follow up to musescore#20634
ottavas, pedal lines, trill lines, staff and system text lines, let ring and palm mute lines Follow up to musescore#20634 Also fix a Visual Studio warning about a potentially ininitialized variable
Resolves: #15321
Changed so that range selections add both front and back segments of spanners.