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.

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

Merged
merged 2 commits into from Oct 31, 2014

Conversation

MarcSabatella
Copy link
Contributor

There are a bunch of related issues I am trying to fix together. The fixes are related enough that I think it best to make and test them together, but I will annotate the changes in the commentsto show which issue is actually fixed by which change.

The underlying problem here is that adjustCanvasPosition is called too often and/or with too broad a target. This is in part because Score objects emit posChanged signals any time you click a note:

emit posChanged(pos, unsigned(tick));

This causes all scoreviews to respond, when really, only the current scoreview usually needs to. Furthermore, there is not enough of the original context (eg, which note was clicked to trigger this) to be able to position the score well. I guess there isn't a more focused call to adjustCanvasPosition in order to maintain the separation between libmscore and mscore. Or may it is intended that in some cases, all scoreviews should respond. In any case, I elected to leave this alone, and instead deal with the problems on the scoreview side.

There are also other places where adjustCanvasPosition is called unnecessarily, or with to broad a target.

@@ -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

else if (sys->page()->systems()->size() == 1) {
// otherwise, just keep current vertical position if possible
// 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 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.

@MarcSabatella
Copy link
Contributor Author

BTW, I can rebase if the PR otherwise looks good. I didn't want to mess up the line comments.

lasconic added a commit that referenced this pull request Oct 31, 2014
@lasconic lasconic merged commit 85b4130 into musescore:master Oct 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants