Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix #301496: voltas excluded from navigation #5755

Merged
merged 1 commit into from Feb 28, 2020
Merged

fix #301496: voltas excluded from navigation #5755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 22 additions & 2 deletions libmscore/navigate.cpp
View file
Open in desktop
Expand Up @@ -575,13 +575,23 @@ Element* Score::nextElement()
else
return score()->firstElement();
}
#if 1
case ElementType::VOLTA_SEGMENT:
#else
case ElementType::VOLTA_SEGMENT: {
// TODO: see Spanner::nextSpanner()
System* sys = toSpannerSegment(e)->system();
if (sys)
staffId = sys->firstVisibleStaff();
}
// fall through
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

In the comment it is clear why you disabled this piece of code but I'm wandering a future reader of this code will have any clue why this code is disabled and it might take some time to figure out.
A small comment like // See PR5755 for why the code is disabled could be very helpfull.

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 generally rely on git blame to show me exactly which commit added the code in question, and also, the referenced comment in nextSpanner() already provides more detail. But I guess it doesn’t hurt to add more still.

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 updated the comment on nextSpanner to add an explicit link back to the issue and the PR, just in case git blame fails (as it might if we go ahead and change the coding style everywhere).

But I elected not to update all the other sites. It's unnecessary duplication. The existing TODO comments already point to the location where the information is. That way if the situation ever changes and we need to update the comment, we only need to do it in one place.

Copy link
Contributor

Choose a reason for hiding this comment

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

To put the comment everywhere is indeed overkill. But the added comment made it clear what's happening. It makes it much easier to read the code without using git blame or searching through the history.
Thanks!

case ElementType::SLUR_SEGMENT:
case ElementType::TEXTLINE_SEGMENT:
case ElementType::HAIRPIN_SEGMENT:
case ElementType::OTTAVA_SEGMENT:
case ElementType::TRILL_SEGMENT:
case ElementType::VIBRATO_SEGMENT:
case ElementType::VOLTA_SEGMENT:
case ElementType::LET_RING_SEGMENT:
case ElementType::PALM_MUTE_SEGMENT:
case ElementType::PEDAL_SEGMENT: {
Expand Down Expand Up @@ -682,13 +692,23 @@ Element* Score::prevElement()
Segment* s = toSegment(e);
return s->prevElement(staffId);
}
#if 1
case ElementType::VOLTA_SEGMENT:
#else
case ElementType::VOLTA_SEGMENT: {
// TODO: see Spanner::nextSpanner()
System* sys = toSpannerSegment(e)->system();
if (sys)
staffId = sys->firstVisibleStaff();
}
// fall through
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

case ElementType::SLUR_SEGMENT:
case ElementType::TEXTLINE_SEGMENT:
case ElementType::HAIRPIN_SEGMENT:
case ElementType::OTTAVA_SEGMENT:
case ElementType::TRILL_SEGMENT:
case ElementType::VIBRATO_SEGMENT:
case ElementType::VOLTA_SEGMENT:
case ElementType::PEDAL_SEGMENT: {
SpannerSegment* s = toSpannerSegment(e);
Spanner* sp = s->spanner();
Expand Down
55 changes: 43 additions & 12 deletions libmscore/segment.cpp
View file
Open in desktop
Expand Up @@ -1177,7 +1177,8 @@ Element* Segment::nextAnnotation(Element* e)
auto ei = std::find(_annotations.begin(), _annotations.end(), e);
if (ei == _annotations.end())
return nullptr; // element not found


// TODO: firstVisibleStaff() for system elements? see Spanner::nextSpanner()
auto resIt = std::find_if(ei + 1, _annotations.end(), [e](Element* nextElem){
return nextElem && nextElem->staffIdx() == e->staffIdx();
});
Expand All @@ -1197,7 +1198,8 @@ Element* Segment::prevAnnotation(Element* e)
auto reverseIt = std::find(_annotations.rbegin(), _annotations.rend(), e);
if (reverseIt == _annotations.rend())
return nullptr; // element not found


// TODO: firstVisibleStaff() for system elements? see Spanner::nextSpanner()
auto resIt = std::find_if(reverseIt + 1, _annotations.rend(), [e](Element* prevElem){
return prevElem && prevElem->staffIdx() == e->staffIdx();
});
Expand All @@ -1212,6 +1214,7 @@ Element* Segment::prevAnnotation(Element* e)
Element* Segment::firstAnnotation(Segment* s, int activeStaff)
{
for (auto i = s->annotations().begin(); i != s->annotations().end(); ++i) {
// TODO: firstVisibleStaff() for system elements? see Spanner::nextSpanner()
if ((*i)->staffIdx() == activeStaff)
return *i;
}
Expand All @@ -1225,6 +1228,7 @@ Element* Segment::firstAnnotation(Segment* s, int activeStaff)
Element* Segment::lastAnnotation(Segment* s, int activeStaff)
{
for (auto i = --s->annotations().end(); i != s->annotations().begin(); --i) {
// TODO: firstVisibleStaff() for system elements? see Spanner::nextSpanner()
if ((*i)->staffIdx() == activeStaff)
return *i;
}
Expand Down Expand Up @@ -1429,8 +1433,22 @@ Spanner* Segment::firstSpanner(int activeStaff)
Element* e = s->startElement();
if (!e)
continue;
if (s->startSegment() == this && s->startElement()->staffIdx() == activeStaff)
return s;
if (s->startSegment() == this) {
if (e->staffIdx() == activeStaff)
return s;
#if 1
else if (e->isMeasure() && activeStaff == 0)
return s;
#else
// TODO: see Spanner::nextSpanner()
else if (e->isMeasure()) {
SpannerSegment* ss = s->frontSegment();
int top = ss && ss->system() ? ss->system()->firstVisibleStaff() : 0;
if (activeStaff == top)
return s;
}
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

}
}
return nullptr;
Expand All @@ -1445,16 +1463,29 @@ Spanner* Segment::lastSpanner(int activeStaff)
std::multimap<int, Spanner*> mmap = score()->spanner();
auto range = mmap.equal_range(tick().ticks());
if (range.first != range.second){ // range not empty
for (auto i = --range.second; i != range.first; --i) {
for (auto i = --range.second; ; --i) {
Spanner* s = i->second;
Element* st = s->startElement();
if (!st)
Element* e = s->startElement();
if (!e)
continue;
if (s->startElement()->staffIdx() == activeStaff)
return s;
}
if ((range.first)->second->startElement()->staffIdx() == activeStaff) {
return (range.first)->second;
if (s->startSegment() == this) {
if (e->staffIdx() == activeStaff)
return s;
#if 1
else if (e->isMeasure() && activeStaff == 0)
return s;
#else
// TODO: see Spanner::nextSpanner()
else if (e->isMeasure()) {
SpannerSegment* ss = s->frontSegment();
int top = ss && ss->system() ? ss->system()->firstVisibleStaff() : 0;
if (activeStaff == top)
return s;
}
#endif
}
if (i == range.first)
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

}
}
return nullptr;
Expand Down
45 changes: 39 additions & 6 deletions libmscore/spanner.cpp
View file
Open in desktop
Expand Up @@ -947,9 +947,28 @@ Spanner* Spanner::nextSpanner(Element* e, int activeStaff)
Element* st = s->startElement();
if (!st)
continue;
if (s->startSegment() == toSpanner(e)->startSegment() &&
st->staffIdx() == activeStaff)
return s;
if (s->startSegment() == toSpanner(e)->startSegment()) {
if (st->staffIdx() == activeStaff)
return s;
#if 1
else if (st->isMeasure() && activeStaff == 0)
return s;
#else
// TODO: when navigating system spanners, check firstVisibleStaff()?
// currently, information about which staves are hidden
// is not exposed through navigation,
// so it may make more sense to continue to navigate systems elements
// only when actually on staff 0
// see also https://musescore.org/en/node/301496
// and https://github.com/musescore/MuseScore/pull/5755
else if (st->isMeasure()) {
SpannerSegment* ss = s->frontSegment();
int top = ss && ss->system() ? ss->system()->firstVisibleStaff() : 0;
if (activeStaff == top)
return s;
}
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again.

//else
//return nullptr;
}
Expand Down Expand Up @@ -979,9 +998,23 @@ Spanner* Spanner::prevSpanner(Element* e, int activeStaff)
while (i != range.first) {
--i;
Spanner* s = i->second;
if (s->startSegment() == toSpanner(e)->startSegment() &&
s->startElement()->staffIdx() == activeStaff)
return s;
Element* st = s->startElement();
if (s->startSegment() == toSpanner(e)->startSegment()) {
if (st->staffIdx() == activeStaff)
return s;
#if 1
else if (st->isMeasure() && activeStaff == 0)
return s;
#else
// TODO: see nextSpanner()
else if (st->isMeasure()) {
SpannerSegment* ss = s->frontSegment();
int top = ss && ss->system() ? ss->system()->firstVisibleStaff() : 0;
if (activeStaff == top)
return s;
}
#endif
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again.

}
break;
}
Expand Down