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

chore: 初回ファイルの取得をbackendの関数として表現するようにする #2524

Conversation

sevenc-nanashi
Copy link
Member

内容

backend.getInitialProjectFilePath()を追加します。

関連 Issue

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

(なし)

その他

これのためにelectron:serveに引数を渡せるような改修をしました。

@sevenc-nanashi sevenc-nanashi requested a review from a team as a code owner February 8, 2025 16:00
@sevenc-nanashi sevenc-nanashi requested review from Hiroshiba and removed request for a team February 8, 2025 16:00
@Hiroshiba
Copy link
Member

PRありです!!

@sabonerune いつもすみません!何か気づいた点あればコメント頂けると心強いです…!

@Hiroshiba Hiroshiba requested a review from Copilot February 8, 2025 21:22

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (4)
  • src/components/App.vue: Evaluated as low risk
  • README.md: Evaluated as low risk
  • src/backend/browser/sandbox.ts: Evaluated as low risk
  • src/backend/electron/main.ts: Evaluated as low risk
@sabonerune
Copy link
Contributor

自分の環境では問題なく動作しました。

Mac環境の場合はどうでしょうか?
Mac環境でコマンドラインからファイルパスを渡した場合open-fileが発生しないような気がしますが…

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.

@sabonerune ありです!!!
macOSで試した感じ、確かにファイルがわたされてせんでした!


良い感じにコードがまとまってるなと思います!!
もっとリファクタリングできそう・・・!!

いっぱいコメントしていますが、すでに確かめるために変更済みのコードがあるのでPR送ります!!
どういう観点があるのかわかると思うので、コメントと見比べてみてください 🙏

vite.config.mts Outdated Show resolved Hide resolved
vite.config.mts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/backend/electron/main.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

すみません。。。。checkout -bしてからPRしようとしてたのですが、完全にミスってPRに直接pushしてしまいました。。。 🙇 🙇 🙇 🙇

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

と言いつついろいろレビューコメントしたのですが、全部変更したプルリクエストを送ります!!
それをレビューいただいて問題無さそうであればこのPRもマージいただければ・・・!!

(勝手にコミットしてしまった点も含め、だいぶややこしいことになってしまってすみません。。)

vite.config.mts Outdated
Comment on lines 103 to 104
const doubleSeparatorIndex = process.argv.indexOf("--");
if (doubleSeparatorIndex !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

ダブルセパレーターというのがなにかわからない・・・かも?
うーん! doubleDashIndexで!!

@@ -245,7 +245,24 @@ function checkMultiEngineEnabled(): boolean {
return enabled;
}

let filePathOnMac: string | undefined = undefined;
function getArgv(): string[] {
Copy link
Member

Choose a reason for hiding this comment

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

ちょっとドキュメント追加させていただきます!

return [];
}

let initialFilePath: string | undefined = getArgv()[0];
Copy link
Member

Choose a reason for hiding this comment

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

これどこで値が変わるのか全くわからない超危険コードなのでカプセル化したいですね。。。
とりあえずTODOコメントだけ足させていただきます!!

Comment on lines 378 to 379
fs.existsSync(initialFilePath) &&
fs.statSync(initialFilePath).isFile() &&
Copy link
Member

Choose a reason for hiding this comment

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

これなぜか前からあった処理だけど、エラーの握り潰しに見える・・・。
うーん!消しちゃいますか!!

@@ -163,6 +160,7 @@ onMounted(async () => {
});

// プロジェクトファイルが指定されていればロード
const projectFilePath = await window.backend.getInitialProjectFilePath();
Copy link
Member

Choose a reason for hiding this comment

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

あー backendに書いたあと、それを叩くactionsを書くのが一応決まりなんですよねー。。
守られてないとこも結構あるし、たぶん明文化されてないけど。。
actions作っちゃいますね!

@@ -163,6 +160,7 @@ onMounted(async () => {
});

// プロジェクトファイルが指定されていればロード
const projectFilePath = await window.backend.getInitialProjectFilePath();
if (typeof projectFilePath === "string" && projectFilePath !== "") {
Copy link
Member

Choose a reason for hiding this comment

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

ここはstringかundefinedしかこないのと、空文字が来ることはなくなったのでifの中身変えたほうが良さそう!

@sevenc-nanashi
Copy link
Member Author

問題なさそうなのでマージします。

@sevenc-nanashi sevenc-nanashi added this pull request to the merge queue Feb 15, 2025
Merged via the queue into VOICEVOX:main with commit 41d6666 Feb 15, 2025
11 checks passed
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