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.

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

feat: 不適切なフォルダにファイルを保存できないようにする #1844

Merged
merged 18 commits into from
Feb 26, 2024

Conversation

sabonerune
Copy link
Contributor

内容

VOICEVOXの更新時にVOICEVOXのインストールディレクトリに保存したファイルが消失したというポストが複数件寄せられています。
これをある程度回避するためにファイルを不適切な場所に保存しようとした場合それを拒否して再度保存場所を指定するようにします。

保存に不適切なディレクトリはとりあえずVOICEVOXのインストールディレクトリと設定ファイルのディレクトリにしました。

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

スクリーンショット 2024-02-13 232842

その他

macでの動作確認をしていません。

直接インストールディレクトリにファイルを置いた場合は無力です。

書き出し先を固定するのチェックをオンにした状態で書き出し先のフォルダが未指定だとVOICEVOXのインストールディレクトリにファイルを保存してしまうという良くない挙動をすることに気づきました。

@sabonerune sabonerune requested a review from a team as a code owner February 13, 2024 14:40
@sabonerune sabonerune requested review from y-chan and removed request for a team February 13, 2024 14:40
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.

書き出し先を固定するのチェックをオンにした状態で書き出し先のフォルダが未指定だとVOICEVOXのインストールディレクトリにファイルを保存してしまうという良くない挙動をすることに気づきました。

・・・・・・インストールディレクトリの保存してしまうの、これが原因なのでは・・・!?

かなり問題だと感じました。
お手数おかけして申し訳ないのですがissueの作成お願いすることはできますか 🙇 🙇 🙇

たぶんディレクトリ名が指定されてない時に空文字列が代入されてて、それが相対パスになっちゃってインストールディレクトリに保存される気がしました。
空のディレクトリを指定できてしまうのがよくなさそうですが、とりあえず応急処置として・・・・・$HOME/Documents辺りのパスが代入されてるといいかも??

src/background.ts Outdated Show resolved Hide resolved
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.

retryShowSaveDialogWhileSafeDirラッパー、良いですね!!

SAVE系のダイアログではretryShowSaveDialogWhileSafeDirを呼ばないといけないっての、忘れそうですね・・・・・・・・。
とはいえ防ぐ方法は思いつかなかったのですが・・・。

もしよかったらいろんな機能追加の際に気を配っていただけると心強いです 🙇

src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
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.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
/**
* 警告ダイアログを表示し、ユーザーが再試行を選択したかどうかを返す
*/
const showWarningDialog = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

📝 true/falseのリターンがどっちがどっちかわかりやすい関数名にする。showRetryOrNotDialog・・・?

src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
src/background.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba removed the request for review from y-chan February 19, 2024 17:50
@Hiroshiba
Copy link
Member

(コメント取り組んでくださったのめちゃくちゃ助かります、ありがとうございます 🙇)

@sabonerune
Copy link
Contributor Author

コンフリクト解消まで終わりました。

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!!!

ありがとうございました!!!
まだまだ微妙な UX なところはいっぱいあると思うのでぜひ改良していただけるととても助かります!!!

SHOW_OPEN_DIRECTORY_DIALOGSHOW_SAVE_DIRECTORY_DIALOGの差がわかりづらそうだったのでコメントを書き足させていただきます!

@Hiroshiba Hiroshiba merged commit f218028 into VOICEVOX:main Feb 26, 2024
8 checks passed
@sabonerune sabonerune deleted the feat/unsafe-save-dir branch February 26, 2024 23:00
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