-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feature player filter #61
Conversation
@kashiken The CI checks failed. Please review the errors and push fixes. |
…dio-preview into feature-player-filter
The CI checks passed. |
@sukumo28 I also fixed the code in this PR. thanks. |
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.
Thank you for the PR!
There are a few things I would like you to fix.
src/webview/components/playerSettings/playerSettingsComponent.ts
Outdated
Show resolved
Hide resolved
src/webview/components/playerSettings/playerSettingsComponent.ts
Outdated
Show resolved
Hide resolved
src/webview/components/playerSettings/playerSettingsComponent.ts
Outdated
Show resolved
Hide resolved
@kashiken The CI checks failed. Please review the errors and push fixes. |
…ialized variables
…hFilterFrequency~ input
Thank you for your feedback. Now, I've finished fixing the code including the bugs I found. Regarding the way of contribution, I think you mentioned this matter at first, I completely agree with your suggestion to make the issue in advance of PR. |
Thank you!! |
The CI checks passed. |
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.
Thank you for your kind PR.
There are a few things I would like you to fix, so please check them.
Thank you for your feedback. Proposed UX goalI think it's better to set the filter frequency independently of analyzer range because fine tuning with cut and try is needed. Why current implementation has conditional branchThere are two reasons. Firstly, I used the CHANGE event of HTML input instead of the INPUT event to reflect the change immediately. Secondly, the bi-quad filter cannot work as expected when the frequency set is too low, especially at zero. How to resolveFrom a UI perspective, I suppose it is better to set the filter frequency independently of analyzer range. Regarding another condition value !== newValue, I think we can just remove this condition. Let me summarize my proposal again:
What do you think about this idea? |
Thank you!! If you read the following, you may think that the UX goal of immediately reflecting value changes for trial and error can be achieved even if you reuse the existing minFrequency and maxFrequency input. However, I also write about a compromise at the end, so please check that if you really want to separate the input fields. About limiting the frequency so that the biquad filter works as expectedIn this regard, if you reuse minFrequency and maxFrequency, the reflection in the UI will not be strict. (If you want the minFrequency or maxFrequency input to become sampleRate/2 instead of the defaultValue when too large an input is entered, you can change the code for the check in the minFrequency or maxFrequency setter. About event loopsWill an event loop occur if you use a CHANGE event? Currently, minFrequency updates setting.minFrequency in the CHANGE event handler. Thus, I don't think a loop would occur even if the CHANGE event was used. About the choice between the CHENGE event and the INPUT eventConsidering that there is no risk of an event loop, the reason for choosing INPUT instead of CHANGE is probably just the difference between whether an event will fire just by changing the value. About the opinion that you want to separate the input fields to allow fine adjustments by hearingThe desire to separate the input fields because fine adjustments are necessary seems to be due to a misunderstanding between us regarding the timing at which changes in minFrequency and maxFrequency are reflected in AnalyzeSetting. If there is a concern, it is that, as mentioned above, INPUT events are more convenient for trial and error. If this is not acceptable, there is no problem in changing minFrequency and maxFrequency to INPUT events. Policy if you absolutely want to separate the input fieldsYou are probably the only user of this function, so if you have read all of these comments and still have a strong desire to separate the input fields, we would like to prioritize the user's (your) wishes. I will write the policy in that case here. As mentioned above, since an infinite event loop will not occur, there is no need to check with an if statement when firing an event such as PS_UPDATE_HPF_FREQUENCY. There is also no need to check with an if statement when reflecting the limited frequency in the UI (even in that case, the event loop will not occur). For convenience, I think using the INPUT event is more convenient when making fine adjustments, so I think INPUT is fine. |
Thank you for the feedback! About event loopI apologize for the incorrect explanation in my last comment. I was also wrong regarding the matter of event loop itself. About input fieldFor this point, I would appreciate it if you could allow me to maintain the current condition. About event typeRegarding the event type, I would like to choose CHANGE event. ModificationFinally, I modified the code as following:
Please confirm above, thank you. |
The CI checks passed. |
Thank you! |
This PR introduces a filter function for the player.
Purpose of Change
Changes
Tests
Notes