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

Piano Roll scroll bar improvement #6818

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Aug 21, 2023

This is a simple quality-of-life improvement for the Piano Roll.

When working with large midi clips in the Piano Roll, it can be tiring to switch between clips because the Piano Roll's horizontal scrollbar always snaps back to the beginning of the clip, forcing the user to scroll over to the position where they want to work every time.

With this PR, double clicking a midi clip in the Song Editor will open that clip in the Piano Roll not to the beginning of the clip but to wherever on the midi clip the user double clicked. Hopefully this is intuitive enough and can help make the main LMMS workflow just a little bit smoother for users.

Partially addresses #2093 by making it easier to navigate to the section of a clip you're interested in from the Song Editor.

Demonstration:
https://github.com/LMMS/lmms/assets/33463986/e95b8b5b-50ab-46e4-af80-4800d99e7665

I also did some minor refactoring.

Closes #7663

include/MidiClipView.h Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
m_leftRightScroll->setGeometry(
m_whiteKeyWidth,
height() - SCROLLBAR_SIZE,
width() - m_whiteKeyWidth,
Copy link
Member

Choose a reason for hiding this comment

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

This suggestion is quite possibly out of scope here, but may also make things easier, so I'll mention it and see what you think. Currently the horizontal scroll bar extends to the very right of the piano roll, below the vertical scroll bar. This is unusual in my experience; what you tend to see is an empty square in the bottom right where each scrollbar stops at the edge of the other. If we followed this pattern (i.e. made the horizontal scrollbar narrower by SCROLLBAR_SIZE), it would mean you wouldn't need to fiddle with the vertical scrollbar width in setScrollbarPos, as well as being more consistent with other software.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try to add that to this PR, since it looks like it shouldn't be too difficult. I can also adjust the Song Editor and Automation Editor scrollbars while I'm at it. The Automation Editor updates its horizontal scrollbar's range in the paint event just like Piano Roll did before I improved it, so I should probably apply the same improvement to that class too.

@superpaik
Copy link
Contributor

It works ok.
Just one observation, for not so big midi clips. If the midi clip is fully visible in the Piano Roll area, I don't see the benefit of moving the scroll bar according to double-click position. I think it's better to see all the clip at once.

@superpaik
Copy link
Contributor

Another thing that would help with large midi clips (and in general) is to have reference inside the PianoRoll to the Song TimeLine because now the reference is "local". I mean if the clip starts on, say, beat 34, in the pianoroll it starts with 1, and with large clips specially it's kind of difficult to keep track of where you really are. It would be helpful to have something like 1(34), 2(35), etc. I know it's not related strictly to this PR, but it would help editing large midi clips.

@messmerd
Copy link
Member Author

@superpaik I implemented your suggestion for clips small enough to fit on the screen.

To do it, I had to modify MidiClip a little so that I could query the exact length of the clip instead of its length when rounded to the nearest bar. Thankfully the calculation needed to find the exact length was already being performed in the process of calculating the length rounded to the nearest bar, so there should not be a performance cost to this.

@superpaik
Copy link
Contributor

It works ok. Thanks.
Just one thing that I believe is not related to this PR but just in case. It works ok for midi clips with standard notes and for no-time notes, but when the clip mixes both tipes of notes, the length of the clip doesn't take in mind the no-time notes. In current master is just like that, when you add no-time notes at the end of a standard notes clip, visually the length of the midi clip is not updated (and I think that internally neither) so the current behaviour.

@superpaik
Copy link
Contributor

Just one thing that I believe is not related to this PR but just in case. It works ok for midi clips with standard notes and for no-time

Maybe #5902 solve this problem.

Copy link
Member

@Spekular Spekular left a comment

Choose a reason for hiding this comment

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

Code LGTM and no issues found when testing! Tried several combinations of zoom levels in piano roll and song editor, as well as different scroll positions in song editor just to be sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automation editor is not opening at current position song / automation play
4 participants