Skip to content

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
Copy link
Contributor

Copilot AI left a comment

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送ります!!
どういう観点があるのかわかると思うので、コメントと見比べてみてください 🙏

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

}

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.

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

});
// プロジェクトファイルが指定されていればロード
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作っちゃいますね!

// プロジェクトファイルが指定されていればロード
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