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

Fix scroll control event handling on the filter dialog. #139

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

tmiw
Copy link
Collaborator

@tmiw tmiw commented Jun 5, 2021

Turns out that the scroll controls on the filter dialog didn't really work. I traced it down to them not being connected to the same events the ones on the main window are.

BTW, I also noticed on the main window that it was connecting the same scroll events multiple times per control, so I have some cleanup for those in this PR too.

@drowe67
Copy link
Owner

drowe67 commented Jun 5, 2021

Hi @tmiw - thanks for this PR. I'm unclear about the bug that this PR addresses. Can you pls describe how I can demonstrate the bug on freedv-gui/master?

@tmiw
Copy link
Collaborator Author

tmiw commented Jun 5, 2021

@drowe67, basically, drag any of the scroll controls (e.g. mic bass gain) in the Filter dialog left or right. Without this PR, the numbers next to each control don't increase or decrease, whereas they do with this PR.

@drowe67
Copy link
Owner

drowe67 commented Jun 5, 2021

So on freedv-gui/master e1e6ad3 this is what I see on my Ubuntu 18 system:

  1. Open Filter Dialog, and working in Mic Equaliser/Bass
  2. Drag Gain to right -> Gain number increases to 11.2
  3. Drag Freq to right -> Freq number increases to 324
  4. In both cases I see the spectrum of the mic EQ change.

I tried it a few times, here's a screen shot:

Screenshot from 2021-06-06 08-36-57

If I understand your bug report correctly, it would suggest the mic and speaker EQ (plus other filtering options) are all broken when you try them? Sorry if I'm missing something here ..... 🤔

@tmiw
Copy link
Collaborator Author

tmiw commented Jun 5, 2021

@drowe67, hmm, I wonder if it's a macOS specific issue, then. It definitely didn't work for me before on my 2019 MBP (Intel) or the M1 Mac Mini.

@drowe67
Copy link
Owner

drowe67 commented Jun 5, 2021

Perhaps something to do with a different wxWidgets version? Or some sort of corner case we have missed to date. This feature has been in pretty constant use since it was written (many years ago) so we would have had a bug report if the issue was wide spread on all platforms.

@tmiw
Copy link
Collaborator Author

tmiw commented Jun 5, 2021

Perhaps something to do with a different wxWidgets version? Or some sort of corner case we have missed to date. This feature has been in pretty constant use since it was written (many years ago) so we would have had a bug report if the issue was wide spread on all platforms.

The latest Windows build is using 3.1 so I would imagine we'd have heard something by now if that was it. Something OS related seems more likely.

@tmiw
Copy link
Collaborator Author

tmiw commented Jun 10, 2021

@drowe67, so yeah, I'm okay with these changes. After this is merged, we can do one more test build if you'd like. However, I haven't really heard of many issues (other than #137 and this one, the latter which hasn't been duplicated by any other non-Mac users to my knowledge), so perhaps we're okay with incrementing version numbers after this?

@drowe67
Copy link
Owner

drowe67 commented Jun 10, 2021

@tmiw OK cool. So this ended up being a MacOS-only issue? It's quite unusual to have different behavior across operating systems with wxWidgets.

Guess we should make sure the changes in this PR are working on Linux and Windows.

@tmiw
Copy link
Collaborator Author

tmiw commented Jun 11, 2021

@drowe67, yep, I can't duplicate the issue on the Windows build of 0fea124 (and you weren't able to duplicate it on Linux either).

I guess the next step is to generate builds based on this PR and email them out?

@drowe67
Copy link
Owner

drowe67 commented Jun 11, 2021

Hi @tmiw how about we merge this one 👍 I tried it on Linux again and it seems to work the same as master in terms of the moving the filter scroll bars. So if it's fixed the macOS behavior, and you've tried Windows then I think we are good.

Re above - yes good idea to bump the version number and make some test builds. I'm keen to have end users give codec2/master a workout so we can make a codec2 release.

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