-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add Continuous Auto-Scrolling #7396
Add Continuous Auto-Scrolling #7396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style review, also pay attention to codefactor.
Btw you forgot classic theme.
Okay, I made some icons for classic, and hopefully I've fixed all the style issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
f13ff3f
to
0a16a93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do testing before merge.
Co-authored-by: saker <[email protected]>
Co-authored-by: saker <[email protected]>
Co-authored-by: saker <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have found a bug that's introduced by this PR, i.e. that is not in master.
Steps to reproduce
- Load a song.
- Set the mode to "Continuous".
- Play the song.
- Quickly change to "Off" and then to "Paged".
The line of the play head line will not move with the play head anymore.
I think I found the problem. The return statement in the simplification of the stepped auto scrolling code exited the function too soon, which happened to skip the code for updating the play head line position. I fixed it by modifying the if statement to not require a return. |
Had a feeling this would break something, my bad. I got tunnel vision on that one piece of code and forgot about the surrounding function. |
I'll test then approve. 👍 |
I think it might be helpful to have more descriptions for the tooltip when toggling between the modes (i.e., you can tell the user what this mode will do in particular rather than simply saying "Auto scrolling", because not all modes auto scroll). |
I added specific tooltips for each button state: "Stepped auto scrolling", "Continuous auto scrolling", and "Auto scrolling disabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, also something minor I noticed
Co-authored-by: saker <[email protected]>
Co-authored-by: Dominic Clark <[email protected]>
I realized that changing the scrolling step size also broke zooming centered around the mouse position. I have now fixed that; just a simple multiplication by |
Just remembered that there was another bug when drawing the rubberband selection rect, where the left side would get teleported to the left as everything scrolled. Once again, a simple division by |
Alright, I think those are all the necessary changes, so unless anyone spots a new issue, this should pr should be ready to merge. |
I have a question about this, and it is that the continuous scrolling is more fluid and consistent in the Piano Roll than in the Song Editor, since in the latter the scrolling stops suddenly and is not constant. I show you this in the following videos: Autoscrolling in Piano Roll Autoscroolling.Piano.Roll.mp4Autoscrolling in Song Editor Autoscroolling.Editor.de.Cancion.mp4Is this on purpose? Or is there some other way to improve this? |
Ah, whoops, I forgot to test it with smooth scroll enabled. I should have set the scroll using For now, you can temporarily fix it by disabling smooth scroll in the settings. |
Description
This pull request adds the option to have auto-scrolling continuously scroll the view to keep up with the playback.
Changes
AutoScrollState
enum changed fromEnabled
/Disabled
toStepped
/Continuous
/Disabled
TimeLineWidget.cpp
is changed from having 2 states (Enabled
/Disabled
) to having 3 states (Stepped
/Continuous
/Disabled
)SongEditor.cpp
andPianoRoll.cpp
, a check is added for whether the mode isStepped
orContinuous
; if it'sStepped
, then is runs the old code for auto-scrolling; if it'sContinuous
, then it snaps the horizontal scroll to halfway behind the time line so that the line appears in the center of the view.SongEditor.cpp
's horizontal scrollbar's functionality was changed to make it scroll by ticks instead of bars. This required modifyingTrackContentWidget.cpp
to be positioned by ticks instead of bars to properly draw the grid background.