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 #32866: score jumping issues #1415

Merged
merged 2 commits into from Oct 31, 2014
Merged

fix #32866: score jumping issues #1415

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d02b701
fix #32866: score jumping issues
MarcSabatella Oct 29, 2014
4b40c8d
revert to old playback behavior for tall systems with >1 on page
MarcSabatella Oct 29, 2014
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
14 changes: 10 additions & 4 deletions mscore/scoreview.cpp
View file
Open in desktop
Expand Up @@ -1369,7 +1369,7 @@ void ScoreView::moveCursor(int tick)

_cursor->setRect(QRectF(x, y, w, h));
update(_matrix.mapRect(_cursor->rect()).toRect().adjusted(-1,-1,1,1));
if (mscore->panDuringPlayback())
if (mscore->state() == ScoreState::STATE_PLAY && mscore->panDuringPlayback())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing us to try to re-position the canvas to the beginning of the measure every time you clicked a note in a measure. But if the measure is too wide to fit on the canvas, that might actually force the clicked note off screen. This code was clearly (?) intended to only work during playback only, but it gets triggered when you click a note as well. See the video in this comment: http://musescore.org/en/node/31156#comment-131201

adjustCanvasPosition(measure, true);
}

Expand Down Expand Up @@ -3634,6 +3634,9 @@ void ScoreView::pageEnd()

void ScoreView::adjustCanvasPosition(const Element* el, bool playBack)
{
if (this != mscore->currentScoreView())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a general catch-all to make sure nothing we do in one scoretab affects the view in the other. Right now, several things force all scoretabs to update position: deleting measures, pasting, and even just clicking a note. The latter issue is fixed more directly elsewhere in this PR, but for the first two cases, this is the best place I found to catch it. Between this and the modification to posChanged() below, I am addressing http://musescore.org/en/node/37676

return;

if (score()->layoutMode() == LayoutMode::LINE) {
if (!el)
return;
Expand Down Expand Up @@ -3751,9 +3754,10 @@ void ScoreView::adjustCanvasPosition(const Element* el, bool playBack)
showRect.setHeight(el->height());
}
else {
// let user control height
// showRect.setY(r.y());
// showRect.setHeight(1);
// otherwise, just keep current vertical position
// see issue #7724
showRect.setY(r.y());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was already in place but commented out in the initial commit. It seems to directly address http://musescore.org/en/node/7724, and as far as I can tell, it works well. It ensures that while playing, we stay at the same vertical position within the score.

Hmm, actually, I just realized, this is not right if there is more than one system on the page but even one system does not fit on the canvas. That will hopefully be rare, but I will try to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This addresses http://musescore.org/en/node/7724 - playback jumping if bottom staff of system is visible on screen. General fix is beyond me, but I can at least do a cheap fix for the case of a single system on the page. This is probably the most common case given this bug only strikes if the full system does not fit on screen - it means the system must be pretty big. The cheap fix is to just keep the same vertical position. Seems to work OK - better than nothing, anyhow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this code is limited to a single system per page. My understanding is that it would work for two systems equally well?

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 tried that :-). What happens is it literally keeps the canvas focuses at the same Y position the page, so even though the playback has moved on to the second system, the canvas stays focuses on the first. There is probably a way to calculate an appropriate offset into the the new system, but it wasn't obvious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok let's merge this. I have something just a little bit better maybe.

      // canvas is not as tall as system
      if (r.height() < showRect.height()) {
            if (sys->staves()->size() == 1 || !playBack) {
                  // track note if single staff
                  showRect.setY(p.y());
                  showRect.setHeight(el->height());
                  }
            else {
                  qreal min = qMax(showRect.y(), r.y());
                  qreal max = qMin(showRect.bottom(), r.bottom());
                  if (min + border < max) {
                        // let user control height
                        showRect.setY(r.y());
                        showRect.setHeight(r.height());
                        }
                  }
            }

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 don't completely follow, but I tried this out, and I'm not sure I understand what is meant to be different about it. It still fails in the case as my code - page with >1 system. But your code looks cleaner - doesn't require separate cases - and in any event, as I mentioned before, the multiple-system-per-page really is not nearly as important as getting the single-system case right. And both versions do this.

BTW, in case it isn't totally clear what the problem case is: create a score with four staves per system and make sure you can fit at least two systems on the page. Zoom way in so only two or three staves fit on screen, position the canvas to view the last staff of the system, click a note on the last staff, and hit play. When the playback hits the end of the system, the canvas does jump to the next system as it should, but it jumps to the top of the next system rather than keeping the display focused on the bottom staff.

Copy link
Contributor

Choose a reason for hiding this comment

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

mm that's how I tested it and it's working for me but only for the first page.

showRect.setHeight(r.height());
}
}

Expand Down Expand Up @@ -5571,6 +5575,8 @@ Element* ScoreView::elementNear(QPointF p)

void ScoreView::posChanged(POS pos, unsigned tick)
{
if (this != mscore->currentScoreView())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also addresses http://musescore.org/en/node/37676 - the issue with clicking in particular. Clicking a note triggers it to playback of that note, which causes the Score object to emit posChanged(), which brings us here. But we only need to act if this is the current scoretab. In addition to the reported bug, it was also the case that if you left one scoretab in note entry mode than when and worked in the other scoretab, the note entry cursor in the first scoretab would jump to follow what you were doing in the second scoretab.

return;
switch (pos) {
case POS::CURRENT:
if (noteEntryMode())
Expand Down