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.

Sign up for GitHub

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

Jump to bottom

サードパーティがエンジンへのアクセス情報を得るための設定書き出し機能 #1765

Merged
merged 21 commits into from
Feb 13, 2024
Merged

サードパーティがエンジンへのアクセス情報を得るための設定書き出し機能 #1765

merged 21 commits into from
Feb 13, 2024

Conversation

nmori
Copy link
Contributor

@nmori nmori commented Jan 26, 2024

内容

  • ポート番号動的変更に伴い、サードパーティツールが通信ポートを特定出来ない問題が発生する
  • この PR では、起動しているエンジンに関する情報をエクスポートする機能を追加する

対応として

  • エンジン起動時、再起動、停止時にサードパーティ向けのデータを書き出す
  • 書き出す場所は AppData/Roaming/VOICEVOX/runtime-info.json (Windows)

関連 Issue

ref #1738

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

その他

nmori added 12 commits February 5, 2023 11:35
@nmori
未対応エンジン追加時にリストが消える件(VOICEVOX#1168)
0972e5e 
・追加されたエンジンが未対応である場合には追加を阻止
・追加されてしまっている場合には、エラーで処理中断しないように
@nmori
lintチェックエラー部分の修正
cb5552c
@nmori
コードレビューの反映 (ref VOICEVOX#1179)
00b7673 
・MinimumEngineManifestの更新
@nmori
コードレビュー分の反映②　ref VOICEVOX#1179
4b21b85 
・engineManifests[selectedId]自体が undefined であるケースに対応
@nmori
Merge branch 'main' of https://github.com/voicevox/voicevox
ad8c1de
@nmori
* コンフリクトの修正
6e91471
@nmori
Merge branch 'main' of https://github.com/voicevox/voicevox
4e8cde0
@nmori
Merge branch 'main' of https://github.com/VOICEVOX/voicevox
3ce28d9
@nmori
Merge branch 'main' of https://github.com/VOICEVOX/voicevox
a72f3b7
@nmori
Merge branch 'main' of https://github.com/VOICEVOX/voicevox
aac80a5
@nmori
Merge branch 'main' of https://github.com/VOICEVOX/voicevox
a37c7a2
@nmori
サードパーティがエンジンへのアクセス情報を得るための設定書き出し機能(ref VOICEVOX#1738)
80008bc 
* ファイルは runtime-info.json に書き出し
* エンジン全起動もしくは個別起動/終了のタイミングで更新
@nmori nmori requested a review from a team as a code owner January 26, 2024 14:04
@nmori nmori requested review from Hiroshiba and removed request for a team January 26, 2024 14:04
sevenc-nanashi
sevenc-nanashi reviewed Jan 26, 2024
View reviewed changes
src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

良いですね！！！！！
処理が簡潔で読みやすかったです！！！
この処理で良いはずと、いろいろなとこ読んでくださったんだなと感じました！

src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
Hiroshiba
Hiroshiba requested changes Jan 26, 2024
View reviewed changes
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.

（すみません、癖でapprove押してしまいました 🙇 ）

@nmori
* 関数名の変更 : writeEngineInfoFor3rdParty
351a3ab 
* 排他ロックの追加
* 処理の非同期化
@nmori
Copy link
Contributor Author

nmori commented Jan 27, 2024 •
edited
Loading

  • 自プロセス向けに、タイムアウト付きロックと非同期関数利用を追記。仮に数回呼ばれてもタイムアウトするのでVOICEVOX側に影響出さずにすむかと思います。
  • JSONの書き込みは、非同期関数が呼ばれてから処理したほうがスムーズかとおもったので、 fs-extra の JSON書き込み関数に差し替えました。
  • サードパーティツールがファイルを開いている時などは書き込みエラーが出ますが、ログにエラー記録が残るだけにしました。（ユーザの操作に伴わないエラーなので、ログだけで問題ないかと考えました）

ちなみに #1753 でファイルパスが変わるというアナウンスがあるので、サードパーティはこのデータを参照するようにという形につながるとよいかとおもいました。

Hiroshiba
Hiroshiba reviewed Jan 29, 2024
View reviewed changes
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です！！

src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
@nmori
* コンストラクタ引数でファイルパスを渡すように
7cff6f0 
* 関数をシンプルに
* ログメッセージ修正
* コメント位置修正
@nmori
Copy link
Contributor Author

nmori commented Jan 30, 2024

レビューありがとうございます。
コメント頂いた観点、理解しましたので織り込んでみました。
よろしくお願いいたします。

Hiroshiba
Hiroshiba reviewed Jan 30, 2024
View reviewed changes
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.

良い感じですね！！

いろいろ細かいことコメントしていますが、もう大枠は完成していると思うので、もしよければ！！

src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background/engineManager.ts Outdated Show resolved Hide resolved
@nmori
* エクスポートファイパスを渡す所を引数にした
c93a819 
* 変数、関数名修正
* いくつかの構造をクラス化
@nmori
Copy link
Contributor Author

nmori commented Feb 1, 2024

ファイルフォーマットの追加、クラス化しました。
クラス化は見様見真似で一旦形にしました。単純さを保つ部分で改良の余地はあるのかもしれません。
レビューよろしくお願いいたします。

@nmori nmori requested a review from Hiroshiba February 9, 2024 18:31
Hiroshiba
Hiroshiba reviewed Feb 11, 2024
View reviewed changes
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.

良い感じに整ってきているのを感じます！

RuntimeInfoManagerを後から拡張しやすいようにちょっと設計を提案してみました！
（微妙そうな点あったらすみません 🙇 ）

src/background.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
@nmori
* ファイル書き出しクラスに機能を集約
94d56f8 
* 変数名、コメントの修正
Hiroshiba
Hiroshiba approved these changes Feb 13, 2024
View reviewed changes
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！！！

色々調整ありがとうございました、ものすごい分かりやすいコード・仕様になったと思います！！！
ちょっと細かいところをこちらでいくつか変更させていただきます！

あ、そういえば案内をどうしようか考えていませんでした！
そちらはissueの方とかで進めたり、別プルリクエストで進められればと思います！！

丁寧な進行ありがとうございました！！
また力をお借りできると非常に心強いです！！！

src/background.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
src/shared/RuntimeInfoManager.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

ちょっと構造を変更させていただいて、テストを追加させていただきました！

@Hiroshiba Hiroshiba merged commit e159b3c into VOICEVOX:main Feb 13, 2024
8 checks passed
@nmori nmori deleted the #1738_ファイル吐き出し branch February 13, 2024 21:55
This was referenced Feb 14, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wappon28dev wappon28dev wappon28dev left review comments

@sevenc-nanashi sevenc-nanashi sevenc-nanashi left review comments

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nmori @Hiroshiba @sevenc-nanashi @wappon28dev