Skip to content

Conversation

@sevenc-nanashi
Copy link
Member

内容

Configをasyncにします。主にスマホ版のためです(@capacitor/preferencesはasyncしかない)

関連 Issue

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

(なし)

その他

(なし)

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner October 23, 2023 12:15
@sevenc-nanashi sevenc-nanashi requested review from y-chan and removed request for a team October 23, 2023 12:15
@Hiroshiba Hiroshiba requested review from Hiroshiba and removed request for y-chan October 23, 2023 16:43
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.

なるほど・・・・・・。Discordで悩まれてたのこれなんですね。
正直なところ結構煩雑性が跳ね上がってる印象を受けましたが、どうすべきかパッとわかんないですね・・・・。

getConfigをasyncにすると、もうどうしてもいたるところでasync汚染が発生してしまうので、これをなんとか同期処理にする方法をいくつか考えました 😇
こういうところがプログラマーの腕の見せ所なので頑張りたい。

  1. initializeされた後にLet configに代入する
let config
initializeConfig().then((obj) => {config = obj})

みたいな感じ。configがinializeするのを待つ関数もいるかも。

  1. electron用に通常BaseConfigを、async用にBasicAsyncConfigを作る
    正直問題の先送り感が。

  2. let configにしてapp.on("ready")の最初でconfig = await getConfig

Comment on lines 152 to -155
const engineManager = new EngineManager({
config,
Copy link
Member

Choose a reason for hiding this comment

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

configをDIする形じゃなくなってますね~
今まではBaseConfig相当のものがあればOKだったのが、ElectronConfigに依存する形になったので、以前よりも密結合になってしまった感じです。

@sevenc-nanashi
Copy link
Member Author

sevenc-nanashi commented Oct 23, 2023

1と3、多分configがundefinedかのチェックが必要になる気がします。

ちょっと考えてみました:

  • initializeされてないconfigを作っておく
  • electronのready時にinitializeする
  • setをasyncじゃなくする:save()をspawnさせる感じ
    • これだけだと保存されない可能性があるので、内部でカウンターを作り、save前(save_とか?)で加算してsaveで減算させる
    • こんな関数を作る:
async waitForSave() {
  while (this.counter > 0) {
    // 別のPromiseにスレッドを譲る:Pythonのawait asyncio.sleep(0)みたいな
    await new Promise((resolve) => setTimeout(resolve, 0))
  }
}
  • 終了前にwaitForSaveする

@Hiroshiba
Copy link
Member

@sevenc-nanashi 良い感じだと思いました!!

雰囲気がLockだなと感じました。vvppManagerで導入したAsyncLockが使えるかも。

Configの役割がわかってきた気がします。どっちかというと名前はConfigManagerのが良いかもと思いました。
(データの部分がConfigかもしれない)

1と3、多分configがundefinedかのチェックが必要になる気がします。

本当はそうなんですよね・・・。winもundefinedチェックしてないしまあ良いかな〜とか思ってました。

let win: BrowserWindow;

@sevenc-nanashi
Copy link
Member Author

んー、lockというよりはRc/Arcだと思います。
まぁConfig自体の管理は3で、save周りは自分のやつにしようかと思います。

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.

configの取り回しはかなりいい感じになったと思います!!
終了シーケンスに関してちょっとお願いがあったので細かいコメントさせていただきました 🙇

ちょっと自分用のメモにタスクリスト書かせてください!

@sevenc-nanashi
Copy link
Member Author

レビューを反映しました。Config -> ConfigManagerもやってもいいかも?

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!!

コメント良い感じですね!!!

Config -> ConfigManager

良いと思います!
その場合dataが真のConfigかもですね。(ConfigTypeでも良さそう)
あとはまあクラス説明ドキュメントがちょっと変わるくらいかなぁ。

変えた場合は再度reviewもらえれば 🙏


📝 終了シーケンス書き換え

@sevenc-nanashi
Copy link
Member Author

ConfigからConfigManagerにしました。

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!!

型がConfigManagerになってる変数や引数の名前はconfigManagerにしてengineManagerと合わせても良いかもですね
終了シーケンス図の書き足しをするついでにこっちで書き換えさせていただきます!!

@Hiroshiba
Copy link
Member

マージします!

@Hiroshiba Hiroshiba merged commit c2b742c into VOICEVOX:main Oct 26, 2023
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