Skip to content

Conversation

@umi1299
Copy link
Contributor

@umi1299 umi1299 commented Sep 12, 2023

内容

エンジンが起動していないときに渡される FAILED_STARTING ステートをハンドリングしてエラーメッセージを表示するようにしました。
ついでに ERROR ステートのハンドリングも追加しました。

関連 Issue

close #1538

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

VOICEVOX-Ver-999-999-999

その他

@umi1299 umi1299 requested a review from a team as a code owner September 12, 2023 08:59
@umi1299 umi1299 requested review from y-chan and removed request for a team September 12, 2023 08:59
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.

遅くなってすみません...!
1箇所、2つの観点でレビューしました!

ご確認いただけると...!

Comment on lines 12 to 16
<div v-if="allEngineState === 'ERROR'" class="waiting-engine">
<div>
エンジンのエラーによって失敗しました。エンジンの再起動をお試しください。
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

2点コメントが...!
まず、ERRORステートは、エンジンが起動(一度READYステートになっている)していたにもかかわらず、異常終了した場合に設定されるステートのようです。
つまり、これが表示された際にはユーザーは作業中である可能性があります。
保存やエンジンの再起動を促せるといえど、強制的にユーザーの作業を中断させる状態がいいかは少し微妙な気がしました。この変更を適用しない場合は再生時に改めてエンジンが異常終了していることを伝えることが可能(現状通り)ですが、どっちがいいのかは私にもわからないです...まあでも現状維持の方がいい気がします...(@Hiroshiba ヒホさんのご意見も伺いたいです...!)

もう一つは、この変更を適用するとして、エンジンのエラーによって失敗しましたというのは少しわかりづらいだけでなく、主語がないため、ユーザーにとって意味がわからないメッセージになっていると思います。エンジンがエラーを起こしましたなどの方が簡潔でいいかなと...!

Copy link
Member

Choose a reason for hiding this comment

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

プルリク&レビューありがとうございます!!

@y-chan
エラーなるほどです!!(問題に気づいていませんでした、めちゃ助かりました 🙇 )
どうすべきかかなり悩みますね。。。とりあえず客観的な意見を列挙するとこんな感じ?

  • 編集中に操作不能になるのはユーザーをだいぶ不安にさせてしまうので避けたほうが良さそう
  • かといって通知しないのもよくなさそう
  • でも現状エンジンがエラーになってるらしいことに気づけるUXにはなってそう
  • ついでにどのエンジンがエラーになっているのか気付けるとなお良さそう

ということでUX的に良いのはこんな感じですかね!

  1. 操作不能にさせず、allEngineStateをwatchしてERRORになったらエラーになったエンジンを探してトーストを出す
  2. ↑でエンジン探すの難しければとりあえずトーストを出す
  3. トーストを出すのが難しそうであれば、とりあえずこの位置に<!-- TODO: ERROR時はトーストで案内 -->とコメントだけ書いておく

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

ERRORステートは、エンジンが起動(一度READYステートになっている)していたにもかかわらず、異常終了した場合に設定されるステート

すみませんこのことを理解していませんでした
現状は再生時にエンジンがエラーになっていることに気づけるようになっているとのことですので、現状維持のままに戻しました🙇‍♂️
トースト通知はやってみようかと思ったのですが色々分からず、TODOコメントを追加させていただきました
ご確認いただけたら幸いです

@umi1299 umi1299 requested review from Hiroshiba and y-chan September 28, 2023 08:35
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です!

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!!
表示も確認してみました、いい感じだと思います!!

プルリクエストありがとうございました!!
VOICEVOXはいつでもウェルカムです、もしまた良かったら別のにもチャレンジしてみていただけると・・・!

あ、トーストに関してですが、変数をwatchして異常な値だったらトーストを出す形になりそうな気がします。
ちょうどエンジンのポートが自動変更された時にトーストを出す実装が近くにあるので参考になるかもです!

// 代替ポート情報の変更を監視
watch(
() => [store.state.altPortInfos, store.state.isVuexReady],
async () => {
// この watch がエンジンが起動した時 (=> 設定ファイルを読み込む前) に発火して, "今後この通知をしない" を無視するのを防ぐ
if (!store.state.isVuexReady) return;
// "今後この通知をしない" を考慮
if (store.state.confirmedTips.engineStartedOnAltPort) return;
// 代替ポートをトースト通知する
for (const engineId of store.state.engineIds) {
const engineName = store.state.engineInfos[engineId].name;
const altPort = store.state.altPortInfos[engineId];
if (!altPort) return;
store.dispatch("SHOW_NOTIFY_AND_NOT_SHOW_AGAIN_BUTTON", {
message: `${altPort.from}番ポートが使用中であるため ${engineName} は、${altPort.to}番ポートで起動しました`,
icon: "compare_arrows",
tipName: "engineStartedOnAltPort",
});
}
}
);

@Hiroshiba Hiroshiba merged commit 693c493 into VOICEVOX:main Sep 29, 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.

エンジンが起動していなくても「データ準備中・・・」が表示される

3 participants