未対応エンジン追加時にリストが消える件(#1168) #1179

nmori · 2023-02-05T02:52:21Z

内容

supported_featuresが見当たらないときの処理を追加
・追加されたエンジンが未対応である場合には追加を阻止
・追加されてしまっている場合には、エラーで処理中断しないように

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

その他

・追加されたエンジンが未対応である場合には追加を阻止・追加されてしまっている場合には、エラーで処理中断しないように

Hiroshiba · 2023-02-05T09:06:56Z

src/components/EngineManageDialog.vue

@@ -261,7 +261,7 @@
            </div>
            <div class="no-wrap q-pl-md">
              <div class="text-h6 q-ma-sm">機能</div>
-              <ul v-if="engineManifests[selectedId]">
+              <ul v-if="engineManifests[selectedId].supportedFeatures">


ここちょっとややこしいのですが、engineManifests[selectedId]がundefinedになる可能性があったりします。（未知のengineIdがあった場合など）
このとき. supportedFeaturesがエラーになってしまって別の不具合になるかもです！

こういうときjavascriptは便利な記法があって、こう書くと大丈夫なはずです。

Suggested change

<ul v-if="engineManifests[selectedId].supportedFeatures">

<ul v-if="engineManifests[selectedId]?.supportedFeatures">

その記法、ESLintでしばってませんでしたっけ

あ、そうでしたっけ。。。となるとこうですかね。。（npm run fmtが必要そう）

Suggested change

<ul v-if="engineManifests[selectedId].supportedFeatures">

<ul v-if="engineManifests[selectedId] && engineManifests[selectedId].supportedFeatures">

レビューありがとうございます。

この部分のループで使われる　selectedId　が engineIcons （≒engineManifests）から生成されるので
「構造自体は必ずあるが、supportedFeaturesはない可能性がある」という想定で書いてみました。

「その構造が崩れている状態は内部DBがすでにおかしいのでリカバリー不要」として
コードを削減する方向で記述してみましたが、ケアしておいたほうがよかったでしょうか？

たしかにengineManifests[selectedId]は必ず存在しそうですね！
プルリクの形そのままでも特に問題なさそうに思いました！

（どちらかというと過去のコードの意図がわからない＝大事な可能性もあるので、まあとりあえずundefinedの可能性があると想定したコードにしたほうが良いかな･･･。みたいな気持ちでした！）

selectedId　が engineIcons （≒engineManifests）から生成されるので

これは違いますね。（225行目のはアイコンを取れるかどうかの確認、アイコンのフォールバック用です）
エンジン管理ダイアログはエンジンが起動してなくても開ける必要があります。

レビューありがとうございます。理解を間違えました。

selectedIdは変数で
・項目を選択したとき(74行目)に selectEngine() (559行目) でセットされる
・初期値は　”” (421行目)
・確認の通信はエンジンに対しておこなっているので、通信が返ってこない可能性はある

この挙動から行くと、状態が変化したときに
一時的にでも変数が一致しない状況は起きうるとおもったので、
レビュー頂いたコードを追加してPRします。

src/background/engineManager.ts

Hiroshiba · 2023-02-05T09:12:53Z

PRありがとうございます！！

コードはほぼ問題ないのかなと思います！
より洗練するための提案コメントをいくつかしてみました。

・MinimumEngineManifestの更新

・engineManifests[selectedId]自体が undefined であるケースに対応

Hiroshiba

LGTM！！

良い感じの着地なのかなと思いました！！
プルリクありがとうございます！！

Hiroshiba

LGTM！！

修正ありがとうございました！！
提案から修正・PR・その後のやり取りまですごくやりやすかったです！！

もしよかったらぜひまたプルリクエストお待ちしています！！
（初心者歓迎ラベルは結構いろいろ探しやすいかもです！）

Hiroshiba · 2023-02-11T03:04:39Z

問題ないと思うのでマージします！

* 未対応エンジン追加時にリストが消える件(#1168) ・追加されたエンジンが未対応である場合には追加を阻止・追加されてしまっている場合には、エラーで処理中断しないように * lintチェックエラー部分の修正 * コードレビューの反映 (ref #1179) ・MinimumEngineManifestの更新 * コードレビュー分の反映②　ref #1179 ・engineManifests[selectedId]自体が undefined であるケースに対応 * サードパーティがエンジンへのアクセス情報を得るための設定書き出し機能(ref #1738) * ファイルは runtime-info.json に書き出し * エンジン全起動もしくは個別起動/終了のタイミングで更新 * * 関数名の変更 : writeEngineInfoFor3rdParty * 排他ロックの追加 * 処理の非同期化 * * コンストラクタ引数でファイルパスを渡すように * 関数をシンプルに * ログメッセージ修正 * コメント位置修正 * * エクスポートファイパスを渡す所を引数にした * 変数、関数名修正 * いくつかの構造をクラス化 * 議論 #1738 に基づき、最小項目の書き出しに変更 * * ファイル書き出しクラスに機能を集約 * 変数名、コメントの修正 * RuntimeInfoManager.tsをブラッシュアップ * EngineManagerとRuntimeInfoManagerを疎結合に * データ構造調整、テスト追加 * Apply suggestions from code review --------- Co-authored-by: Hiroshiba <hihokaruta@gmail.com>

* 未対応エンジン追加時にリストが消える件(#1168) ・追加されたエンジンが未対応である場合には追加を阻止・追加されてしまっている場合には、エラーで処理中断しないように * lintチェックエラー部分の修正 * コードレビューの反映 (ref #1179) ・MinimumEngineManifestの更新 * コードレビュー分の反映②　ref #1179 ・engineManifests[selectedId]自体が undefined であるケースに対応 * ref #1738 の会話にあった「情報ファイルに関するドキュメント」（新規執筆） * markdown lint でエラーが出てた件の修正 * Update docs/サードパーティ開発者の方へ.md Co-authored-by: Hiroshiba <hihokaruta@gmail.com> * フォーマット表記提案サジェストの適用 Co-authored-by: Nanashi. <sevenc7c@sevenc7c.com> * * 表の表記をJSONP内コメント追記に * VOICEVOXの仕組みを追記 * 環境変数の修正 Co-authored-by: Hiroshiba <hihokaruta@gmail.com> * コメントいただいた部分を中心に追記 * Update docs/サードパーティ開発者の方へ.md --------- Co-authored-by: Hiroshiba <hihokaruta@gmail.com> Co-authored-by: Nanashi. <sevenc7c@sevenc7c.com>

* 未対応エンジン追加時にリストが消える件(#1168) ・追加されたエンジンが未対応である場合には追加を阻止・追加されてしまっている場合には、エラーで処理中断しないように * lintチェックエラー部分の修正 * コードレビューの反映 (ref #1179) ・MinimumEngineManifestの更新 * コードレビュー分の反映②　ref #1179 ・engineManifests[selectedId]自体が undefined であるケースに対応 * 貢献者ガイドラインを明文化　(ref #1190) * レビュー結果の反映① * 着手周りの手順追記 * CONTRIBUTING.md　として配置変更 * markdownlint のエラーを修正 * * ローカル実行時の markdownlint 検索範囲を修正 * Issueを閉じるタイミングを追記 * * ドラフトプルリクエストについての追記 * フォーマットの修正 * * プルリクエストの表記を英語に。 * WIPに付いてのトーンを弱めに。 * リンク切れの修正 *　「その他」の追記 * * レビュー内容の反映 * * e2e部分の追記 * インデント修正 * 提案いただいた分のコミットと追記 * ・査読分の反映・README.mdに誘導リンクを追加 * Apply suggestions from code review * フォーマットを整える * 崩れてしまった部分を戻す * こう？ * なぜか * に戻っていた * pythonはコメントアウトが // ではなかった --------- Co-authored-by: Hiroshiba <hihokaruta@gmail.com>

未対応エンジン追加時にリストが消える件(VOICEVOX#1168)

0972e5e

・追加されたエンジンが未対応である場合には追加を阻止・追加されてしまっている場合には、エラーで処理中断しないように

nmori requested a review from a team as a code owner February 5, 2023 02:52

nmori requested review from Hiroshiba and removed request for a team February 5, 2023 02:52

lintチェックエラー部分の修正

cb5552c

Hiroshiba reviewed Feb 5, 2023

View reviewed changes

src/background/engineManager.ts Outdated Show resolved Hide resolved

nmori added 2 commits February 5, 2023 20:05

コードレビューの反映 (ref VOICEVOX#1179)

00b7673

・MinimumEngineManifestの更新

コードレビュー分の反映②　ref VOICEVOX#1179

4b21b85

・engineManifests[selectedId]自体が undefined であるケースに対応

nmori mentioned this pull request Feb 5, 2023

MinimumEngineManifest格納時の検証を Zodで行う #1181

Closed

3 tasks

Hiroshiba approved these changes Feb 6, 2023

View reviewed changes

nmori added 2 commits February 10, 2023 02:14

Merge branch 'main' of https://github.com/voicevox/voicevox

ad8c1de

* コンフリクトの修正

6e91471

Hiroshiba reviewed Feb 11, 2023

View reviewed changes

Hiroshiba merged commit 82f9751 into VOICEVOX:main Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

未対応エンジン追加時にリストが消える件(#1168) #1179

未対応エンジン追加時にリストが消える件(#1168) #1179

nmori commented Feb 5, 2023

Hiroshiba Feb 5, 2023 •

edited

Loading

sevenc-nanashi Feb 5, 2023

Hiroshiba Feb 5, 2023 •

edited

Loading

nmori Feb 5, 2023

Hiroshiba Feb 5, 2023 •

edited

Loading

sevenc-nanashi Feb 5, 2023

nmori Feb 5, 2023 •

edited

Loading

Hiroshiba commented Feb 5, 2023

Hiroshiba left a comment

Hiroshiba left a comment

Hiroshiba commented Feb 11, 2023

	<ul v-if="engineManifests[selectedId].supportedFeatures">
	<ul v-if="engineManifests[selectedId]?.supportedFeatures">

	<ul v-if="engineManifests[selectedId].supportedFeatures">
	<ul v-if="engineManifests[selectedId] && engineManifests[selectedId].supportedFeatures">

未対応エンジン追加時にリストが消える件(#1168) #1179

未対応エンジン追加時にリストが消える件(#1168) #1179

Conversation

nmori commented Feb 5, 2023

内容

関連 Issue

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

その他

Hiroshiba Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

sevenc-nanashi Feb 5, 2023

Choose a reason for hiding this comment

Hiroshiba Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

nmori Feb 5, 2023

Choose a reason for hiding this comment

Hiroshiba Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

sevenc-nanashi Feb 5, 2023

Choose a reason for hiding this comment

nmori Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Hiroshiba commented Feb 5, 2023

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba left a comment

Choose a reason for hiding this comment

Hiroshiba commented Feb 11, 2023

Hiroshiba Feb 5, 2023 •

edited

Loading

Hiroshiba Feb 5, 2023 •

edited

Loading

Hiroshiba Feb 5, 2023 •

edited

Loading

nmori Feb 5, 2023 •

edited

Loading