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: マウスホイールでパラメータ調整ができないバグ(#1437)を修正 #1440

Merged
merged 7 commits into from
Aug 3, 2023

Conversation

thiramisu
Copy link
Contributor

内容

#1437 の修正PRです。
Vue力不足により、再生と保存にしっかり反映されること以外大丈夫なのかよく分かっていません…。
唯一機能していたモーフィングの実装を真似したのですが、ダメそうだったらcloseしていただいて他の方にお任せしようかと…!

関連 Issue

close #1437

スクリーンショット・動画など

_VOICEVOX.-.Ver.999.999.999.2023-07-31.07-30-02.mp4

その他

@thiramisu thiramisu requested a review from a team as a code owner July 30, 2023 22:33
@thiramisu thiramisu requested review from y-chan and removed request for a team July 30, 2023 22:33
@thiramisu
Copy link
Contributor Author

thiramisu commented Jul 31, 2023

自分の見立てではpreviewSliderHelper.tsには直接値を変更する機能はなく、スクロール時はonChange(value)を呼び出すだけのように見えました。
ついでに発見した不具合として、onChangeにPromiseを返す関数を渡さないと処理中の値の再セットの抑制が上手くできなさそうでした。現状最終的には全部store.dispatchを呼んでおり、Promise<void>を返せないものはなさそうだったので、その点を変更し、型定義箇所でも明示しました。
(現状だと300msのdebounce中に終わらない処理はないと思うので問題なさそうでしたが…)
ついでにmaxもオプションから必須プロパティにしました。

@thiramisu thiramisu changed the title マウスホイールでパラメータ調整ができないバグを修正 Fix: マウスホイールでパラメータ調整ができないバグ(#1437)を修正 Jul 31, 2023
Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

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

LGTMです!
なんで今まで問題なく動いていたのか謎ですね...

@thiramisu
Copy link
Contributor Author

thiramisu commented Aug 2, 2023

あ、発生原因を書き忘れていたので一応書き残しておきます。
0.14.7リリース直後のリファクタ(#1322)でリファクタされすぎてしまったのが原因みたいでした。
値を直接入力した時や、スライダーを直接ドラッグで操作した時とは別経路なので、多分そこら辺と混同してしまったのかと思います。

あとJSDocをミスっていることに気が付いてしまったので、そのコメントアウトだけ修正しました。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

onChangeの処理とhandleParameterChangeで状態変更の流れが2つあるので、統一した方が良さそうに感じました!
(ちなみにhandleParameterChangeの方だけ値調整のadjustが呼ばれる感じでした)

ただはまあ問題はないと思うので、ちょっと勝手ながらFIXMEコメントだけ追加させていただきます・・・!

src/components/AudioDetail.vue Show resolved Hide resolved
@@ -65,7 +80,7 @@ export const previewSliderHelper = (props: Props): PreviewSliderHelper => {
// Reactive references of each props
const modelValue = computed(props.modelValue);
const min = computed(() => (props.min && props.min()) ?? 0);
const max = computed(() => (props.max && props.max()) ?? 100);
Copy link
Member

Choose a reason for hiding this comment

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

ただのメモですが、元々デフォルトパラメーターが100に設定されてたのは、quasarのデフォルト値がそうだったからだったりします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

そうだったんですね!それならまあどちらでも良かったかもです。

@Hiroshiba Hiroshiba merged commit 81fbf39 into VOICEVOX:main Aug 3, 2023
@thiramisu thiramisu deleted the fix-audio-info-slider-scroll branch August 4, 2023 01:37
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.

マウスホイールでパラメータ調整欄のスライダーを調整できない
3 participants