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

[WIP] Horizontal Zoom #83

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

[WIP] Horizontal Zoom #83

wants to merge 10 commits into from

Conversation

trirpi
Copy link
Contributor

@trirpi trirpi commented Oct 5, 2023

This PR introduces horizontal scrolling using ctrl+scroll as raised in #36.

@DamRsn
Copy link
Owner

DamRsn commented Oct 7, 2023

Hey @trirpi,

Thanks for starting this. Is this already functional or still a work in progress?

Couple comments on the coding style (sorry this should be documented in a Contributing.md file)

  • Member variables should have an appended m at the beginning (mZoomLevel, mMaxZoomLevel ...)
  • Make sure to format your code with clang-format (using the _clang-format configuration file provided in the repo)
  • Local variable should be named in snake case (my_local_variable)

Also I think (not sure) that mouseWheelMove be marked override?

What would be great for this as well, is that it would work with track-pad zoom movement (I'm on Mac and I'd love to have it work in addition to the ctrl/cmd + wheel). But I don't how easy it is to do that with JUCE, I've never done it myself

@trirpi trirpi changed the title Horizontal Zoom [WIP] Horizontal Zoom Oct 7, 2023
@trirpi
Copy link
Contributor Author

trirpi commented Oct 7, 2023

Hey @trirpi,

Thanks for starting this. Is this already functional or still a work in progress?

Couple comments on the coding style (sorry this should be documented in a Contributing.md file)

* Member variables should have an appended m at the beginning (`mZoomLevel`, `mMaxZoomLevel` ...)

* Make sure to format your code with clang-format (using the `_clang-format` configuration file provided in the repo)

* Local variable should be named in snake case (`my_local_variable`)

Also I think (not sure) that mouseWheelMove be marked override?

What would be great for this as well, is that it would work with track-pad zoom movement (I'm on Mac and I'd love to have it work in addition to the ctrl/cmd + wheel). But I don't how easy it is to do that with JUCE, I've never done it myself

It's still WIP, I've marked it as such now.

Thanks a lot for the feedback, I'll incorporate it! @override is a good idea since it is a virtual function that is overwritten.

There is a JUCE event handler for the track pad zoom movement, so that should be quite easy to add once I finish this.

@trirpi
Copy link
Contributor Author

trirpi commented Nov 26, 2023

Seems to work quite well now. One enhancement would be keeping the same place in view when zooming. So, I'll still try to do that.

It might also be nice to have normal scroll, perform a horizontal scroll on the combined audio region.

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