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

WIP: 読み方&アクセント辞書ダイアログの右側パネルを別コンポーネントにする #2290

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jdkfx
Copy link
Contributor

@jdkfx jdkfx commented Oct 10, 2024

内容

  • 辞書の単語・読み、アクセントなどの追加・編集を行う部分を新しいコンポーネントに移動

関連 Issue

close #2234

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

その他

関連: #2237

@Hiroshiba
Copy link
Member

リファクタリング本当にありがたいです!!!
分からないとこあったら何でもお聞きいただけると・・・! 🙏

@jdkfx
Copy link
Contributor Author

jdkfx commented Oct 10, 2024

まだ作業中段階ではありますが、新規に編集部分のコンポーネントを作成しました。
元からあったコンポーネントと、新規に作成したコンポーネントの双方で同じ処理を行っている部分に関しては新たなコンポーザブルとして切り分けました。

ただ、今のところこの状態で動かすとUIなどの表示部分が動いていない模様で、その原因の追求と改善を行っていきたいと考えております。

@jdkfx
Copy link
Contributor Author

jdkfx commented Oct 12, 2024

dictionaryManageDialogOpenedComputedをコンポーザブルに渡してあげているのですが、そのせいか辞書ダイアログを読み込んだ際にエンジンの読み込みが終わらないという挙動が起きており、その原因がわかりません。
dictionaryManageDialogOpenedComputedを定義は以下のようになっています。

const dictionaryManageDialogOpenedComputed: WritableComputedRef<boolean>

この変数をコンポーザブルに渡す際、

型 'WritableComputedRef<boolean>' の引数を型 '() => number' のパラメーターに割り当てることはできません。
型 'WritableComputedRef<boolean>' にはシグネチャ '(): number' に一致するものがありません。

というエラーが出ておりました。
computed()の型付けなども考えてみましたが、うまくいきませんでした。
https://ja.vuejs.org/guide/typescript/composition-api#typing-computed

@Hiroshiba
Copy link
Member

エンジンの読み込みが終わらないという挙動が起きており、その原因がわかりません。

読み込み中かどうかの判定にはloadingDictStateを使ってますが、これをコンポーザブルに渡してないから値が変わらないためですね!!

今回の場合はコンポーザブルにするかどうかはどちらでも良いはずで、先にコンポーネントを切り出してあげた方がやりやすいかもです。
その後にコンポーザブルを使ってリファクタリングを行うかどうか、となるかも・・・!

参考になれば!!

@jdkfx
Copy link
Contributor Author

jdkfx commented Nov 30, 2024

スクリーンショット 2024-11-30 23 28 50
  • modelValuebooleanの値が入るべきなのだが、辞書単語の読みの値(String)が入ってしまっている。
  • それ以外にも、型の不一致エラーによってエディターコンポーネントが開かないという意図せぬ挙動が起きてしまっている。
  • シングルコメントを残したのだが、uiLockedに関係しているのではないかと考えている。
現状出ているエラーについて
23:24:25 [vite] hmr update /components/Dialog/DictionaryManageDialog.vue

 ERROR(vue-tsc)  Property 'computeRegisteredAccent' does not exist on type '{ wordPriority: Ref<number, number>; userDict: Ref<Record<string, UserDictWord>, Record<string, UserDictWord>>; voiceComputed: ComputedRef<...>; ... 8 more ...; toWordEditingState: () => void; }'.
 FILE  /Users/jdkfx/Desktop/workspace/voicevox/src/components/Dialog/DictionaryEditWordDialog.vue:351:3

    349 |   setYomi,
    350 |   createUILockAction,
  > 351 |   computeRegisteredAccent,
        |   ^^^^^^^^^^^^^^^^^^^^^^^
    352 |   loadingDictProcess,
    353 |   discardOrNotDialog,
    354 |   cancel,

 ERROR(vue-tsc)  Argument of type 'Ref<string, string>' is not assignable to parameter of type 'Ref<"loading" | "synchronizing" | null, "loading" | "synchronizing" | null>'.
  Type 'string' is not assignable to type '"loading" | "synchronizing" | null'.
 FILE  /Users/jdkfx/Desktop/workspace/voicevox/src/components/Dialog/DictionaryEditWordDialog.vue:362:3

    360 |   surface,
    361 |   uiLocked,
  > 362 |   yomi,
        |   ^^^^
    363 |   accentPhrase,
    364 |   surfaceInput,
    365 | );

 ERROR(vue-tsc)  Argument of type 'WritableComputedRef<boolean, boolean>' is not assignable to parameter of type 'Ref<string, string>'.
  Types of property 'value' are incompatible.
    Type 'boolean' is not assignable to type 'string'.
 FILE  /Users/jdkfx/Desktop/workspace/voicevox/src/components/Dialog/DictionaryManageDialog.vue:226:3

    224 |   uiLocked,
    225 |   loadingDictState,
  > 226 |   dictionaryManageDialogOpenedComputed,
        |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    227 | );
    228 | </script>
    229 |

[vue-tsc] Found 3 errors. Watching for file changes.
[ESLint] Found 0 error and 0 warning

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.

ちょっと見てみてました! これかなり難しいですね!!!

たぶんなのですが、コンポーザブルなしでリファクタリングを考えるほうがずっと単純になりそうな予感がしました。
コンポーザブルは「別々のコンポーネントで別々のインスタンスなどを作る」ためのものですが(再利用性が高い)、今欲しいのは「別々のコンポーネントで同じインスタンスを作る」(共有)なので、目的と少しずれてるかもです。

ちなみに共有する場合はContextをInject/Provideすると良い感じになることがあります。
https://ja.vuejs.org/guide/components/provide-inject
とりあえず一旦これはやらないで、一旦変数や関数をひたすら子コンポーネントに渡すのが良い気がします。


とりあえずの方針として、まずUI系だけをコンポーネントから切り出し、saveとかの操作用の関数は全部親から子に渡すのはどうでしょうか? 👀
ぱっと仕分けしてるので間違ってるかもですが、雰囲気こんな感じかなと

  • 子コンポーネント側
    • HTML構造全部
    • 音声再生機構
      • nowGenerating, nowPlaying, play, stopとか
    • メニュー系
      • surfaceInput, yomiInputとか
    • アクセント系
      • changeAccent
  • 親コンポーネント側
    • uiLock、createUILockAction
    • wordEditing
    • 状態変数とそのsetter
      • surfaceとかsetSurfaceとか
      • accentTypeの状態変数がなぜか無いので作ると良さそう
    • 関数いろいろ
      • saveWord, deleteWord, resetWordとか
      • あとステートの移動用の関数とか
      • 子コンポーネントに移動できるのもありそうだけど、不明

@jdkfx
Copy link
Contributor Author

jdkfx commented Dec 2, 2024

@Hiroshiba

ちなみに共有する場合はContextをInject/Provideすると良い感じになることがあります。

初めて知りました!inject/provide見てみます!

とりあえずの方針として、まずUI系だけをコンポーネントから切り出し、saveとかの操作用の関数は全部親から子に渡すのはどうでしょうか? 👀

少し前のコミットの戻して対応してみます!

@Hiroshiba
Copy link
Member

参考になれば…!
コードをがっつり読んでみてわかったのですが、これは切り分けかなり難しいなと感じました。。
不明な点あれば結構気軽に聞いていただければ!!🙏

@jdkfx
Copy link
Contributor Author

jdkfx commented Dec 3, 2024

ぱっと仕分けしてるので間違ってるかもですが、雰囲気こんな感じかなと

ここで仕分けられている関数などは、仕分けられている親・子に移動or実装と言う形であっていますでしょうか?

@Hiroshiba
Copy link
Member

あ、そのニュアンスで書いていました!
親は現DictionaryManageDialog.vue、子はDictionaryEditWordDialog.vueみたいな想定です!
(ちなみに全然間違ってるところもあると思います、間違ってたらすみません。。 🙇 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants