-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow waveform zoom levels to be fractional for smoother zooming with mouse wheel #1843
Conversation
…eel. Zoom level is still saved in cfg as an index, so the stored value is still an integer.
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.
Welcome back and thanks for this nice little change.
@@ -111,7 +111,8 @@ void DlgPrefWaveform::slotUpdate() { | |||
midVisualGain->setValue(factory->getVisualGain(WaveformWidgetFactory::Mid)); | |||
highVisualGain->setValue(factory->getVisualGain(WaveformWidgetFactory::High)); | |||
normalizeOverviewCheckBox->setChecked(factory->isOverviewNormalized()); | |||
defaultZoomComboBox->setCurrentIndex(factory->getDefaultZoom() - 1); | |||
// Round zoom to int to get a default zoom index. | |||
defaultZoomComboBox->setCurrentIndex(static_cast<int>(factory->getDefaultZoom()) - 1); |
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.
Can this result in a situation where a user accidentally has their zoom level changed slightly by rounding because they press Apply in the preferences?
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.
The preferences only controls the default zoom level, so hitting apply only updates the saved value in the config. It doesn't change the zoom level if the user has used the mouse wheel to change the zoom from the default. Changing the selection in the preferences does update the zoom level to those values. All in all the experience seems ok.
It used to be possible to right click on the waveform to reset to the default zoom level. I don't know why that was removed, would it be ok to add it back in? Right click-and-hold is used for fast track navigation so I guess that's a little overloaded.
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.
Okay, thanks for clarifying. Sorry for the delay reviewing this. I'm going over old PRs now that we finally got 2.2.0 released.
It used to be possible to right click on the waveform to reset to the default zoom level. I don't know why that was removed, would it be ok to add it back in? Right click-and-hold is used for fast track navigation so I guess that's a little overloaded.
Seems reasonable to me. I didn't know about the right click and drag action for quick seeking. I don't really understand the use case considering it's much easier to seek where you want by simply clicking on the overview waveforms. If you want to implement this, please open a new PR.
@ywwg I've been waiting for this and only just noticed it got merged. It's beautiful! Trying to zoom waveforms on a Mac used to be a nightmare before, but this works really nicely. Thanks for doing it. |
Zoom level is still saved in cfg as an index, so the stored value is still an integer.