feat: Windowsのアンインストーラーでインストールしたエンジンを削除できるようにする #1518
Conversation
これどうすべきか本当に迷っているのですが、そもそもアプリアンインストール時にプラグインは消去されるべきなんでしょうか。。。 |
|
個人的な考えとしては
ボイロ・アイボスは音声ライブラリそれぞれがPCにインストールされるソフトウエアだからそのようになっていると思います。 Windows アプリケーション開発 - ベスト プラクティス とはいえ全部削除してしまうのはあまり良くないかもしれませんね… |
|
設定ファイルが原因の場合に問題が解決するというのは、それはそうなのですが、やはり誤って消してしまうことを防ぎたい気持ちが強めです。 Windowsのベストプラクティス、なるほどです。このようなことが書かれていました。
ちょっと可能かどうかわからないのですが、アンインストーラーを起動してすぐに、デフォルトがオフの「設定ファイルも削除する」と、デフォルトがオンの「追加エンジンを削除する」などのチェックボックスを表示し、ボタンとして「アンインストール」「キャンセル」などがあるようなダイアログを表示するとかはどうでしょうか。 |
|
アンインストール前に差し込めそうなフックはありませんでしたが、アンインストール完了後に差し込めそうなフックはありました。 とりあえずそこで削除できるか試してみます。 |
|
なるほどです！！ |
|
とりあえずページを追加してみました。
|
|
@sabonerune すごく良さそうに感じました！！！！！ 色々ありがとうございます！！！ 追加エンジンの削除は割と気軽に再インストール可能なので、再確認ダイアログはなくせるかもと思いました（デフォルトでオンですし）。 |
|
とりあえずそのように変更しました。 |
There was a problem hiding this comment.
試しているときに気づいたのですがエンジンのファイルが大きいと削除に時間がかかりUIが応答なしになってしまうようです。
なるほどです･･･！！ これは結構しょうがないのかなと思いました。
設定ディレクトリを吹き飛ばすの、かなり怖いなと思いました･･･。理由が2点あります。
別でコメントした通り、環境変数などの指定を間違えていたりすると、その親ディレクトリを吹き飛ばしてしまう可能性があるためです。
まあでもこれは結構気をつけることによって避けられるのではとはちょっと思ってます。
もう一つが、将来的に設定ディレクトリに別の何かを保存する可能性もあって、設定ディレクトリが消去される可能性があるのがWindowsのみになっているため、何か問題があったりすることに気づきにくいというのがありそうです。
なので全部の設定ディレクトリを吹き飛ばすのはやめて、ホワイトリスト形式で吹き飛ばすのはどうでしょうか。
具体的には今目的としているのが設定ファイルの消去とvvpp-enginesディレクトリの消去だと思うので、その2つだけにするのが良いのかなと感じました。
（この辺り手探りで進めている状況もあって、何度も変更お願いしてしまって本当に申し訳ないです･･･。 🙇 ）
build/installer.nsh
Outdated
| ${NSD_CreateCheckBox} 0 0 100% 12u "全ての設定ファイルを削除する" | ||
| Pop $removeAllUserDataCheckBox | ||
|
|
||
| ${NSD_CreateCheckBox} 0 13u 100% 12u "追加エンジンのみ削除する" | ||
| Pop $removeAdditionalEngineCheckBox | ||
| ${NSD_Check} $removeAdditionalEngineCheckBox |
There was a problem hiding this comment.
ちなみになのですが、該当ディレクトリがあった場合のみこのチェックボックスを表示する（あるいはdisable/enableを制御する）とかって可能そうでしょうか･･･？
というのも9割9分の人はこのチェックボックスの意味がわからないので、意識から外してあげるとUXが向上するのかなとか思った次第です。
There was a problem hiding this comment.
%APPDATA%/voicevoxがないというのは「インストールはしたが一度も起動したことがない」しか思いつかないですね…
ただ、簡単に処理できそうです。
問題はエンジンの方で起動時に自動的に%APPDATA%/voicevox/vvpp-enginesが作成されます。
また、一度でも追加操作をすると.tmpディレクトリが作成されるのでそれが空であるかも確認が必要で結構コードが複雑になる気がします。
There was a problem hiding this comment.
%APPDATA%/voicevox/vvpp-enginesの中身が空の.tmpディレクトリ以外がない場合は追加エンジンのみ削除するのチェックボックスが無効化されるようにしました。
また、%APPDATA%\voicevoxが空のエンジンディレクトリ以外存在しない場合(または存在しない場合)はページ自体をスキップするようにしました。
There was a problem hiding this comment.
ありがとうございます、すごく良い仕様だと思います！！
VvppManager側の追加エンジンの仕様が変わると、追加エンジン有無の判定ロジックがバグってしまうことに気づきました。
一番確実なのはアンインストーラーのテストを書くことですが、ちょっと現実的ではなさそうに思います 😇
なのでVvppManager側のテストとして、この追加エンジンの判定ロジックと同様のものを実装し、変わってないことを保証し、変わってしまったことを検知したらアンインストーラーのここの処理も変更する必要があることを開発者に知らせるというのはどうでしょう･･･？
とはいえVvppManagerのテストは存在せず、かつ他のユニットテストの実装すらないので、実装するのはちょっとだけ難しいかもしれません。
もしできそうであれば挑戦してみて、難しそうであればとりあえずVvppManager側の追加エンジンディレクトリ周りにFIXMEコメントを書く、というのはどうでしょう 🙇
削除してもよいファイルやディレクトリを一つずつ削除していきディレクトリが空なら削除するような感じでしょうか? ただ、Chromiumが生成する
思いついたのが
ですね。 どれもあまりいい気がしませんが。 |
あ、目的が「追加エンジンの消去」と「設定ファイルの消去」だと考えていたので、これら2つだけをホワイトリスト形式で削除することを考えていました！
一旦 electronで とりあえずこちらのプルリクエストでは主目的から外れるからそのままにしておいて、これらの課題点と解決策の案をissueにまとめるというのはどうでしょう。 |
build/installer.nsh
Outdated
| ${Locate} "$APPDATA\$%VITE_APP_NAME%\logs" "/L=F /M=????????_??????_error.log /G=0" un.removeLogFile | ||
| RMDir "$APPDATA\$%VITE_APP_NAME%\logs" |
There was a problem hiding this comment.
この辺りもそうなのですが、実装側の仕様が変わってディレクトリの場所やファイル名が変わったりしてしまうと、この辺りが意図した通りに動かなくなってしまう可能性があるように感じています。
できればここが正しく動かないときに気づけるようにするのが良いだろうなとは感じているのですが、どれぐらい重きを置けばいいかで結構迷っています･･･。
@sabonerune さん的にどう考えられていますか 👀
There was a problem hiding this comment.
- アンインストーラーなので実行時エラーは避けたい
- ただし未知のファイルの削除は可能な限り避ける
ということを考えて作成しました。
正しく動かないときに気づく方法は思いつかないですね…
ある程度予防する方法なら
%APPDATA%/voicevoxディレクトリが残っている場合はダイアログで手動削除を案内する- 逆に削除してはいけないものを消してしまった場合は気づくことができない
sessionDataを残す場合はこの方法は使えない
- ドキュメントで
userDataに保存するファイルを明示する- 気づかなければ意味がない
- ディレクトリ名やファイル名のパターンを別の場所で定義してエディタ上で検証、NSISの削除指定もそれを使う
- そもそも実現できるか不明
- できたとしても難しかったりNSISスクリプトが肥大化してメンテナンスできなくなる気がする。
等を考えましたがどれも微妙ですね…
There was a problem hiding this comment.
実行時エラーは避けたい
ただし未知のファイルの削除は可能な限り避ける
なるほどです！確かにおっしゃる通りだと思います。予防策としてはなんだかんだ
ドキュメントでuserDataに保存するファイルを明示する
こちらが手軽で、必要な人は情報が（頑張れば）得られるという意味で一番いいのかなと思いました。
軽く調べたのですが多分electronで
という感じですね。 |
あすみません！
これって変更前のデータを消すのは難しいという話ですよね。 |
build/installer.nsh
Outdated
| ; "engine_manifest.json"があるか確認してから削除する。 | ||
| IfFileExists "$R9\engine_manifest.json" 0 +3 |
There was a problem hiding this comment.
この辺りのif文は将来正しく動かない可能性がありそうですが、仮に正しく動かなかったとしてもチェックボックスが出ないだけ＝ユーザーが想定外の挙動を経験することがないので、正しく動かない可能性はそこまで問題ではなさそうに思いました！
build/installer.nsh
Outdated
| ${If} $removeAllUserDataCheckBoxState == ${BST_CHECKED} | ||
| MessageBox MB_YESNO|MB_ICONEXCLAMATION "全ての設定ファイルを削除します。本当によろしいでしょうか。" IDYES +2 | ||
| Abort | ||
| ${EndIf} |
There was a problem hiding this comment.
すみません、ちょっと意図の伝え方がまずかったのですが、設定を全部吹き飛ばすのを聞くダイアログはなくていい想定でいました。
こんな感じのロジックでどうでしょう･･･？
- setting.jsonを消すかどうかのチェックボックス
- 名称は「設定ファイルを消す」など
- デフォルトでオフ
- 追加エンジンを消すかどうかのチェックボックス
- 追加エンジンがあればチェックボックス自体を表示する
- デフォルトでオン
- それ以外のファイルは現状で消しても良いやつは問答無用で消す
- 消してはいけないのは将来的に設定が保存されそうなファイル
- localStorageやCookieなど
- それ以外は消しちゃってもOK
- 消してはいけないのは将来的に設定が保存されそうなファイル
There was a problem hiding this comment.
一度userDataに配置されるファイルとディレクトリを整理します。
%APPDATA%
├── voicevox
│ ├── config.json (electron-store)
│ ├── window-state.json (electron-window-state)
│ ├── logs
│ │ └── ????????_??????_error.log
│ │
│ └── vvpp-engines
│ ├── .tmp
│ │ └── ??????????????
│ │ └── 展開中のエンジンのファイル(恐らくエラー等で中断して残ってしまったもの)
│ │
│ └── *+????????-????-????-????-????????????
│ └── エンジンのファイル
│
└── voicevox-[cpu|cuda]
└── logs (electron-log)
現在の確認ダイアログでOKを選択した後のコードは
追加エンジンのみ削除するを選択した場合vvpp-engines下のファイルとディレクトリをを削除全ての設定ファイルを削除するを選択した場合voicevoxとvoicevox-[cpu|cuda]下のファイルとディレクトリを削除- どちらの場合でも上記の表にないファイル（
sessonDataや未知のファイル）は削除しないまま残す
としています。
- それ以外のファイルは現状で消しても良いやつは問答無用で消す
- 消してはいけないのは将来的に設定が保存されそうなファイル
これが難しいと思います。
There was a problem hiding this comment.
整理ありがとうございます！！
なるほどです、window-state.jsonとかもありましたね･･･。
他にもsessionDataなどがある感じでしたっけ。
- それ以外のファイルは現状で消しても良いやつは問答無用で消す
- 消してはいけないのは将来的に設定が保存されそうなファイル
これが難しいと思います。
ここちょっと難度感をわかってなくて知りたいのですが、どこに設定が保存されているかわからないから難しい、ということでしょうか･･･？ 👀
There was a problem hiding this comment.
ここちょっと難度感をわかってなくて知りたいのですが、どこに設定が保存されているかわからないから難しい、ということでしょうか･･･？ 👀
そうですね。
sessionDataのファイルがドキュメント化されていないので削除するとしたら%APPDATA%\voicevoxを丸ごと削除するしかないが、そうすると将来的に設定が保存されそうなファイルも一緒に消してしまうという感じです。
There was a problem hiding this comment.
なるほどです。
sessionDataの保存先を変更すれば、変更後のデータはアンインストールできるかも？
ただこの場合でも今までのデータは消去できませんが、どうしようもない気がします。
ベータ版の段階で気づけたのでまだ良かったかなと（言い訳ですが･･･）。
他のOSのこともあるし、Q&Aで案内とかしてもいいかもですね。。
この場合、設定ファイルも削除するのは危険な気がします。 一度、設定ファイルの削除は諦めてエンジンの削除だけ実装にしてもいい気がしてきました。 |
あれ、これってなぜでしたっけ 🙇
こちらの方針もありだと思います！ |
|
ではエンジンの削除のみに変更しようと思います。 |
|
@sabonerune 承知です！！良いと思います！ |
削除するエンジンが存在するかの判定を厳密化 $R0はFunctonとのやり取りに使うパターンが多かったので$0に変更 その他コードの整理
sabonerune
changed the title
There was a problem hiding this comment.
1つコメントしましたがほぼlgtmです！！！
勉強になりました、ありがとうございました！
思ったのですが、UX的に考えるとエンジンをインストールし終わったタイミングで、アプリのアンインストールではエンジンが消えないから手動でアンインストールしてもらうことを案内する手もあったのかなと思いました。
この方法ならアンインストーラーの複雑性をあげなくてすみ、OSごとの差分も吸収でき、Q&Aを偶然見に行かなくてもわかるのかなと。
まあもちろん人はエンジンのパージを忘れてアプリをアンインストールしがちだとは思いますが、こういう解決策もあったかなとか思いました。
（後でissueを建てようかなと思います）
Co-authored-by: Hiroshiba <[email protected]>
|
|
There was a problem hiding this comment.
LGTM！！！
マルチエンジン周り・メンテナンス周りということで共有メンションです 🙏
@y-chan @sevenc-nanashi
（もしよかったらレビューコメントいただけるとすごく心強いです 🙇 ）
内容
Wndowsのアンインストーラーで
設定ディレクトリを削除することで設定とインストールしたVVPPエンジンを削除できるようにします。関連 Issue
既に使用していないUI
スクリーンショット・動画など
その他
いくつか削除できないものがあります。
%LOCALAPPDATA%voicevox-engine）議論が必要と思われる点
特に中途半端に削除すると完全に削除したと勘違いさせるリスクがある。
electron-builder.config.jsで.env.productionから環境変数を読み込んでいるが他のファイルからも読み取るべきか？