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

DlgPrefEQ: fix loading/saving main EQ parameters #4884

Merged
merged 5 commits into from
Aug 10, 2022

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Aug 8, 2022

https://bugs.launchpad.net/mixxx/+bug/1983764
https://bugs.launchpad.net/mixxx/+bug/1983789 likely a regression from effects refactoring

Verify lp:1983764 is fixed:

  • load 'Graphic EQ'
  • click 'Reset Parameter' (don't touch sliders)
  • 'frequency image' and output level should not change
  • restart Mixxx
  • 'frequency image' and output level should still be unaffected

Verify lp:1983789 is fixed:

  • load 'Parametric EQ'
  • click 'Reset Parameter'
  • load 'Graphic EQ' (all sliders are centered, don't touch!)
  • close & restart Mixxx
  • all sliders should still be still be centered

Unfortunately, that pref page is still a mess, most (all?) selections are immediately applied (okay) and stored to config (not okay since is clicking Cancel does not reset options to prevous state).
Eventually I'll port #4667 to main one day.

@github-actions github-actions bot added the ui label Aug 8, 2022
@ronso0 ronso0 added this to the 2.4.0 milestone Aug 8, 2022
@ronso0
Copy link
Member Author

ronso0 commented Aug 8, 2022

Remaining issues:

  • scrolling over the main EQ combobox switches the effect, effectively clearing the previous configuration
  • scrolling over main EQ parameter sliders moves the slider but doesn't change the effect parameter

Preferrable fix:
ignore all scroll events sent to sliders & comboboxes, exactly as in Sound Hardware page

@ronso0
Copy link
Member Author

ronso0 commented Aug 9, 2022

Preferrable fix: ignore all scroll events sent to sliders & comboboxes, exactly as in Sound Hardware page

Done, ready for review.

Comment on lines 556 to 557
double paramValue = double(value - slider->minimum()) /
double(slider->maximum() - slider->minimum());
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason the division of int / int did not return a double but int.
Casting dividend & divisor to double fixes that 🤷

Copy link
Member

@daschuer daschuer Aug 10, 2022

Choose a reason for hiding this comment

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

Yes, the compiler keep the type. Once one of the values is double, the result is also double.
Just use static_cast<double>() here to be more in line with the other code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, done.

@daschuer
Copy link
Member

@ferranpujolcamins Thank you for review. My comments are also solved.
Thank you @ronso0 for noticing and fixing the issue.
I think we need urgently branch out a Beta that we receive more Test.

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.

3 participants