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

FIX: electron-storeの保存場所をGPU版と統一する #1005

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

sabonerune
Copy link
Contributor

内容

関連 Issue

close #337

その他

最初はapp.setName()でアプリ名を操作するつもりでしたがそれだけでは保存ディレクトリが変更されなかったのでuserDataのディレクトリをvoicevoxと指定しまうことにしました。

開発版のログが製品版のディレクトリに保存されてしまう問題もついでに修正されました。

electron-logの既知の問題によりデフォルトのログのディレクトリが必ず作成されてしまいます。

}
log.transports.file.resolvePath = (variables) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return path.join(logPath, variables.fileName!);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

electron-logのドキュメントにはElectronの保存場所を使用する場合は次のコードを使うように書かれているが

return path.join(variables.electronDefaultDir, variables.fileName);

https://github.com/megahertz/electron-log/blob/v4.4.1/docs/file.md#resolvepath-variables-pathvariables-message-logmessage--string
electron-logをインポートした時点で設定されていた値が使われるようなので現在のコードになっている。

@sabonerune sabonerune marked this pull request as ready for review November 3, 2022 05:23
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!!

あれですね、設定ファイルの場所が変わるから、以前の設定ファイルをコピーするようなマイグレーションが必要ですね・・・!
とても大事なので別途issueを立てて仕様を決められればと思います!

@sabonerune
Copy link
Contributor Author

app.setPath(
"userData",
path.join(app.getPath("appData"), `voicevox${isDevelopment ? "-dev" : ""}`)
);

https://www.electronjs.org/ja/docs/latest/api/app#appsetpathname-path

パスとして存在しないディレクトリが指定された場合、Error が投げられます。 その場合、そのディレクトリを fs.mkdirSync や類似のもので作成するべきです。

とあるのでこれから修正します。

@Hiroshiba Hiroshiba self-requested a review December 21, 2022 10:17
@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!!

Comment on lines +47 to +54
const fixedUserDataDir = path.join(
app.getPath("appData"),
`voicevox${isDevelopment ? "-dev" : ""}`
);
if (!fs.existsSync(fixedUserDataDir)) {
fs.mkdirSync(fixedUserDataDir);
}
app.setPath("userData", fixedUserDataDir);
Copy link
Member

Choose a reason for hiding this comment

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

fixedUserDataDirが下1000行のどこかで意図せず使われるかもなので、blockにして使えなくするのとかどうでしょう。

Suggested change
const fixedUserDataDir = path.join(
app.getPath("appData"),
`voicevox${isDevelopment ? "-dev" : ""}`
);
if (!fs.existsSync(fixedUserDataDir)) {
fs.mkdirSync(fixedUserDataDir);
}
app.setPath("userData", fixedUserDataDir);
{
const fixedUserDataDir = path.join(
app.getPath("appData"),
`voicevox${isDevelopment ? "-dev" : ""}`
);
if (!fs.existsSync(fixedUserDataDir)) {
fs.mkdirSync(fixedUserDataDir);
}
app.setPath("userData", fixedUserDataDir);
}
}

あるいは関数化するのも良いかもです。
(そのままでも問題ないです!)

@Hiroshiba
Copy link
Member

@y-chan さんもレビューお願いできると!!

こちらがマージされた後は #1008 に取り組んで、あとボイボ準拠エディタ開発者の方々に念のため報告する感じになるかなと思ってます!

@Hiroshiba Hiroshiba requested a review from y-chan January 4, 2023 21:51
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...!
マージしちゃいますね...!

@y-chan y-chan merged commit 7c71421 into VOICEVOX:main Jan 5, 2023
@sabonerune sabonerune deleted the fix/electron-store_directory branch January 6, 2023 16:37
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.

CPU版のelectron-storeの保存先がvoicevox-cpuなのをGPU版と合わせたい
3 participants