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

Canvas jumping issues #3635

Merged
merged 3 commits into from
Apr 30, 2018
Merged

Canvas jumping issues #3635

merged 3 commits into from
Apr 30, 2018

Conversation

MarcSabatella
Copy link
Contributor

We get complaints from time to time about the canvas jumping during note input or other editing operations. Obviously some of this is intended and good, but there have been a few cases identified where I agree we are doing too much. See https://musescore.org/en/node/271271 for a recent forum discussion leading to this PR.

This PR is submitted against 2.3 because the whole canvas positioning seems more funamentally broken in master - I think maybe the width of the page in continuous view is wrong and throwing a lot of things off? But FWIW, the exact same code does compile and works as well as can be expected in master as well.

There are a few distinct changes here, separated into three different commits.

The first commit fixes something that is unquestionably a bug, a regression introduced in 2.2 that affects operations near the right margin in page view with vertical stacking of pages.

The second commit makes the canvas positioning more conservative in continuous view. Previously, any operation near either margin would cause an immediately jump all the way to the other side. This could make sense in some situations but really doesn't in others, as discussed in the forum post above. My changes here prevent the score jumping all the way to the right upon an operation near the right margin or all the way left upon an operation near the left margin. The only big jump I retain is during note entry, where the canvas will still reposition to the left when you near the right edge of the screen. Even here, though, I made it less aggressive, putting the target element 25% from the left edge of the screen rather than 5%. The idea is to keep more of the previous context in view. I also amended it to guarantee the previous and current note both remain in view, so even if we reduce that 25% back down to 5%, there will still be more context than before.

The final commit is a trial I don't recommend keeping as is, and there is a TODO in the code for this. Basically, I turned the "Pan score during playback" toggle button into "Pan score", period. So when disabled, there is no automatic canvas positioning whatsoever (at least, none done by adjustCanvasPosition). Some people swear they want that. I have my doubts, but I figure, get them a test build to see.

I welcome feedback! Probably best to discuss the behavior in the forum, the code here.

@lasconic
Copy link
Contributor

There seems to be a small consensus around the last commit. I don't think changing the icon is that urgent, we could even keep it but we need to change the text for sure. Could you do that and squash it in the last commit together with an issue number?

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented Apr 26, 2018

OK, will do. But what about the continuous panning also discussed in the thread? There seemed to be some support for that as well in the forum thread, but I think maybe this could also be tied to the pan button behavior. I guess that can wait for later, though.

@MarcSabatella
Copy link
Contributor Author

Updated. The second commit fixes the issue in playback mode that my original attempt created, as discussed in forum (cursor stays pinned to right margin during playback) by reverting to something more like the previous behavior of jumping all the way left, but here too is made more conservative by not jumping earlier than necessary. I also added code for continuous scrolling but left it commented out.

@lasconic
Copy link
Contributor

Ok. Will merge. Looking forward for some more heated discussion on the forum :)

@lasconic lasconic merged commit e448965 into musescore:2.3 Apr 30, 2018
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.

2 participants