fix: デフォルトエンジンの「フォルダを開く」ボタンが表示されている問題を修正 #2696
Conversation
|
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ： |
| disableWhenUiLocked: false, | ||
| subMenu: removeNullableAndBoolean([ | ||
| engineInfo.path != "" && { | ||
| Boolean(engineInfo.path) && { |
There was a problem hiding this comment.
falsyだったらという意図を持たせたいという意図もあったりしますかね？ 👀
これは防御的プログラミングをするのではなく、そもそも型的にそういうのが来ないようにプログラミングするのを徹底するのが良い気がしました･･･！！
防御的プログラミングはきりがないので。。
どちらかというと、このバグは防御的プログラミングをしてなかったたからではなく、""に意味をもたせたのが良くなかったということかなぁと。
PRのタイトルを叶えるなら、こうするのが良い気がしました！！
| Boolean(engineInfo.path) && { | |
| engineInfo.isDefault && { |
これが変数の意味と意図が一致している状態で、なるべくこういうコードになるよう目指せればよいのかな～と思いました！！
偉そうなこと言ってますが、僕はこのengineInfo.path != ""の意図を忘れていたので、デフォルトエンジンを意図してると翻訳してくださったの助かりました。。。
おかげで良い提案ができました！！
There was a problem hiding this comment.
自分もisDefaultの方が良さそうな気がします。
There was a problem hiding this comment.
将来的にデフォルトエンジンがvvppでインストールされるようになる可能性を考えるとengineInfo.path != undefinedの方がいいような気がします。
There was a problem hiding this comment.
vvppだった場合も開けないほうが良いのかなと思いました！
ちょっとフォルダを開く機能をなぜ入れたのか覚えていないのですが、「消せるようにするため」だったらこれかなーと。
There was a problem hiding this comment.
確かspeaker_infoをいじるCoeiroinkのような、エンジンのフォルダに対して色々やりたいと言うエンジンに対応するためのはずです。
There was a problem hiding this comment.
なるほどです！
であればまあ、isDefaultで良さそうな気がしますね！
|
僕の提案で @sabonerune さんと @sevenc-nanashi さんがOKそうならマージしてOKかなと思いました！！ |
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
LGTM！！
提案ではengineInfo.isDefaultの反転忘れてました。。ありがとうございます！！
Hiroshiba
requested review from
sevenc-nanashi
and removed request for
sevenc-nanashi
sevenc-nanashi
left a comment
sevenc-nanashi
left a comment
There was a problem hiding this comment.
変更自体は特に問題なさそう。
ぼんやり：何らかの都合（フォークした時とか、ライブラリ管理とか？）でエンジンが開きたくなることがあるかもしれないので、本来はデフォルトでも開けるようにしてもいいのかな〜ってちょっと思いました。
|
@sevenc-nanashi |
3 participants
内容
デフォルトエンジンの「フォルダを開く」ボタンが表示されていました。
デフォルトエンジンのフォルダは開くことができないので表示されないようになっていたはずなので修正します。
スクリーンショット・動画など
その他
engineInfo.pathには空文字列が入ることはないと思うのですが念のため空文字列になっても非表示になるようにしておきました。