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

Zooming with mouse wheel center #3835

Merged
merged 3 commits into from
May 9, 2018
Merged

Zooming with mouse wheel center #3835

merged 3 commits into from
May 9, 2018

Conversation

Premik
Copy link
Contributor

@Premik Premik commented Sep 28, 2017

Fixes the #2303

@Umcaruje
Copy link
Member

Umcaruje commented Sep 29, 2017

Hi Premik, thanks for contributing to LMMS, I tested this out and it works well, and the code is by the guidelines, but you should implement this in the automation editor as well, so we have consistent UX over the software.

@Premik
Copy link
Contributor Author

Premik commented Sep 30, 2017

Hi Umcaruje,
Done

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

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

Tests fine

Copy link
Member

@Umcaruje Umcaruje left a comment

Choose a reason for hiding this comment

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

@Premik Just tested this, the automation editor logic does not behave as expected and like the other two editors, it's actually quite random:
gifrecord_2017-10-03_003512

In comparison, the behavior of the piano roll:
gifrecord_2017-10-03_003614

@Umcaruje Umcaruje added this to the 1.3.0 milestone Oct 2, 2017
Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

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

Right. I retested the PR and found issues. Like @Umcaruje but reversed. Zooming worked well for Automation Editor but not for Piano Roll. I eventually saw the same issues in the Automation Editor but I don't see a pattern in there.

@Premik
Copy link
Contributor Author

Premik commented Oct 3, 2017

@Umcaruje I've noticed this too. But it is another bug or missfeature which was there before my change. Somehow the automation track doesn't expand to new bars as the control poins are being added (like the other editors do). And if you zoom in a bit more the Automation editor won't let you scroll after the Pattern end (I bet it would be the Bar no. 4 in your case). Try with shift+mouse wheel for example. You can workaround this by expanding the Pattern in the song editor.

Maybe a picture would be better. Below the first automation track with the long Pattern is ok. But the second one is dodgy.
automationscroll

@zonkmachine Can you provide more details about that Piano Roll issue?

@PhysSong
Copy link
Member

According to the QAbstractSlider documentation, it looks like a problem with scrolls' range.

@Premik
Copy link
Contributor Author

Premik commented Oct 16, 2017

@PhysSong True. The zooming would center to the cursor as far as it is possible to do so. I think that is where the confusion came from. For example when zooming to some bars at the very beginning or very end of a clip those can never be in the middle of the screen (where the mouse would be).
This works exactly the same way as selecting the zoom level from the top-left drop down first and then scrolling with the scroll-bar. And I don't think it is something that should be changed.

For the automation editor I suggest to open separate ticket and not mix it with this one. As those are not related.

@tresf
Copy link
Member

tresf commented Nov 15, 2017

@Umcaruje, this PR shows 2 changes requested. Is there anything remaining before merging?

Copy link
Contributor

@teeberg teeberg left a comment

Choose a reason for hiding this comment

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

Just gave this another go after merging with master and seems to work well, given the above mentioned constraints.

@zonkmachine
Copy link
Member

@Umcaruje I've noticed this too. But it is another bug or missfeature which was there before my change. Somehow the automation track doesn't expand to new bars as the control poins are being added (like the other editors do). And if you zoom in a bit more the Automation editor won't let you scroll after the Pattern end (I bet it would be the Bar no. 4 in your case). Try with shift+mouse wheel for example. You can workaround this by expanding the Pattern in the song editor.

Could have been this I saw. If the rest of you are happy with this then go ahead and merge it.

@zonkmachine zonkmachine dismissed their stale review January 7, 2018 00:09

Glitch seem to be not related to this PR

@Wallacoloo
Copy link
Member

Just merged this locally and gave it a try - impressively it's still working fine with no merge conflicts even after 8 months. It's undeniably better than what we have in place right now - I feel we can do whatever further work is needed to improve zoom behavior in the automation editor and piano roll sometime down the road after merging.

@Umcaruje Github has this marked as not ready to merge because "Umcaruje requested changes". This is a useful Github feature that I don't want to blow off - but you didn't reply back in November when tresf asked if you were happy to merge it. I'll give you another chance to speak up, otherwise I'm going to merge this in a few days.

@Umcaruje
Copy link
Member

Umcaruje commented May 9, 2018

Sorry about this, I completely forgot about this PR. OP’s explanation for the behavior i expirienced is good I’m merging.

@Umcaruje Umcaruje merged commit e8b69b9 into LMMS:master May 9, 2018
@Wallacoloo
Copy link
Member

@Umcaruje No worries. Thanks for reviewing it.

@Premik Thanks for the contribution! (And sorry we let it sit here inactive/unmerged for so long >.<)

@Premik Premik deleted the ZoomWheel branch May 20, 2018 13:42
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Horizontal mouse-wheel zooming. Ensure zoom center is always on the current mouse position.

* Horizontal zoom using mouse wheel center on the mouse position. For the SongEditor too.

* Wheel center on the Automation editor too.
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.

None yet

8 participants