fix #301496: voltas excluded from navigation #5755
Conversation
| staffId = sys->firstVisibleStaff(); | ||
| } | ||
| // fall through | ||
| #endif |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| staffId = sys->firstVisibleStaff(); | ||
| } | ||
| // fall through | ||
| #endif |
| return s; | ||
| } | ||
| #endif | ||
| } |
| #endif | ||
| } | ||
| if (i == range.first) | ||
| break; |
| return s; | ||
| } | ||
| #endif | ||
| } |
| return s; | ||
| } | ||
| #endif | ||
| } |
Resolves: https://musescore.org/en/node/301496 Alt+Left/Right commands were skipping voltas because we were checking start element and checking against the active staff, but the start element is actually the measure for voltas. The main change here is to go ahead and visit the volta if the active staff if you are navigating the top staff. Arguably, it could make sense to check for the top *visible* staff, since that is what the volta at least appears to be attached to. So I have code here to that. But I disabled it because in practice, neither the navigation commands themsevles nor the screen reader treat invisible staves specially. So a blind user navigating would have no way of knowing the top staff is not visible. So they would likely continue to see it as relevant. I would not the same issue occurs for system text, which we always treat as attached to the top staff only. I added a TODO to indicate where this code would need updating. Eventually we could consider coming up with some way of presenting information about hidden staves. Perhaps in conjunction with a facility allow user to hide staves on specific systems only, which seems to be a fairly common request.
MarcSabatella
force-pushed
the
301496-volta-navigation
branch
from
a91414c
to
ad3b5c2
Compare
Resolves: https://musescore.org/en/node/301496
Alt+Left/Right commands were skipping voltas because
we were checking start element and checking against the active staff,
but the start element is actually the measure for voltas.
The main change here is to go ahead and visit the volta
if the active staff if you are navigating the top staff.
Arguably, it could make sense to check for the top visible staff,
since that is what the volta at least appears to be attached to.
So I have code here to that.
But I disabled it because in practice,
neither the navigation commands themsevles nor the screen reader
treat invisible staves specially.
So a blind user navigating would have no way of knowing
the top staff is not visible.
So they would likely continue to see it as relevant.
I would not the same issue occurs for system text,
which we always treat as attached to the top staff only.
I added a TODO to indicate where this code would need updating.
Eventually we could consider coming up with some way
of presenting information about hidden staves.
Perhaps in conjunction with a facility allow user
to hide staves on specific systems only,
which seems to be a fairly common request.
(Test added)