-
Notifications
You must be signed in to change notification settings - Fork 313
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: プロジェクトの複製を保存する機能を追加 #2571
Conversation
🚀 プレビュー用ページを作成しました 🚀 更新時点でのコミットハッシュ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
機能は良さそうな気がするのですが、重複実装と関数名の違和感があったのでコメントしました!
どうすれば良いかの指針が必要であればコメントください!
(できればその辺りも名無し。さんにできるようになっていただけるとVOICEVOX全体のパワーアップになるので、挑戦してみてもらえると嬉しいです・・・!!!!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
良い関数切り出しですね!!
1点ちょっと気をつけないといけない良い例があったのでコメントしました。
ちょっとそもそもリファクタリングしてみたのを作ってみたので、このPRがマージされたあとにPR出そうと思います!
「複製を保存」(save a copy)のが一般的かも!
https://x.com/i/grok/share/D70YqQ5GBG9rFqL7i3jlMh3SN
関数名はSAVE_AS_COPYでも別に良いかも。
だけど「プロジェクトを」のあとに続けると「複製として保存」が良さそう。
うーーーーーーーーん。「プロジェクトの複製を保存」に変えさせていただきます!!!
また後で戻すかもです。。
src/store/project.ts
Outdated
const result = await context.actions.SAVE_PROJECT_FILE_AS_COPY({ | ||
filePath, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SAVE_PROJECT_FILE
の中でSAVE_PROJECT_FILE_AS_COPY
を呼んでますが、実はこれ危なかったりします。
これらの関数は今偶然コードが一致しているだけで、役割が異なるはずなためです。
例として、「複製を保存」のときだけ特別な処理、例えば「複製を{path}に保存しました!」というダイアログを表示する変更を後で加えたくなったとします。
この処理は関数SAVE_PROJECT_FILE_AS_COPY
にとって不自然ではないので、後から実装する人は問題ないと思って実装します。
でも実はこの関数はSAVE_PROJECT_FILE
から依存されているので、例えば上書き保存したときも「複製を保存しました!」というダイアログが表示されてしまうバグに繋がるんですね。
もう少し抽象的な見方をすると、はSOLID原則のO、オープン・クローズドの原則(コードは拡張に開いていて、修正に閉じるべき)に反します。
「拡張に開く」は「コード変更は簡単にできるよう作るべき」の意味で、
「修正に閉じる」は「コード変更したときに他のものに影響を与えないべき」の意味です。
SAVE_PROJECT_FILE_AS_COPY
のコードはクローズド原則に反して、この関数には問題ない変更をした場合もSAVE_PROJECT_FILE
をバグらせてしまうんですね。
この原則を意識する方法は簡単で、関数を名前(=目的)に背いた使い方をしなければOKです。
「SAVE_PROJECT_FILE_AS_COPY」は「複製を保存する」関数で、SAVE_PROJECT_FILE
は複製
を保存したいわけじゃないはず。
なのでたとえば今のSAVE_PROJECT_FILE_AS_COPY
と全く同じ処理をする関数を切り出し、「プロジェクトファイル未指定ならファイル選択ダイアログを開き、指定したファイルに書き込んで、エラーが起きたらダイアログを表示する」ような名前を付ければ共通化してOKとなります。
ちなみにこの関数の名付けは難しいんですよね。
こういうときは大抵より良い共通化ができたりします。
マージ後にPR送るので見ていただければ!!
とりあえず一旦FIXMEコメントを書くのでどうでしょうか。
const result = await context.actions.SAVE_PROJECT_FILE_AS_COPY({ | |
filePath, | |
}); | |
// FIXME: SAVE_PROJECT_FILE_AS_COPYへの依存をやめる | |
const result = await context.actions.SAVE_PROJECT_FILE_AS_COPY({ | |
filePath, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds a “Save as Copy” functionality to the project, allowing users to save a duplicate version of the current project file without changing the original project's file path.
- Added a new store action (SAVE_PROJECT_FILE_AS_COPY) in the project store for saving a project copy.
- Updated existing SAVE_PROJECT_FILE, added PROMPT_PROJECT_SAVE_FILE_PATH and WRITE_PROJECT_FILE actions to support the new functionality.
- Updated hotkey settings and the menu bar to include a shortcut and button for saving a project copy.
Reviewed Changes
File | Description |
---|---|
src/store/project.ts | Added new actions for saving a project copy and refactored save logic. |
src/domain/hotkeyAction.ts | Updated hotkey definitions to support the new copy saving action. |
src/store/type.ts | Updated type definitions for the new actions. |
src/components/Menu/MenuBar/MenuBar.vue | Added a new menu item and hotkey registration for saving a project copy. |
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/store/project.ts:255
- [nitpick] If the intention is to preserve the current project file path when saving as a copy, consider adding a comment to clarify this behavior to avoid confusion in future maintenance.
action: createUILockAction(async (context, { filePath: filePathArg }) => {
src/domain/hotkeyAction.ts:159
- [nitpick] Verify that the hotkey combination for 'プロジェクトの複製を保存' does not conflict with other shortcuts and follows platform-specific conventions.
action: "プロジェクトの複製を保存",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
内容
複製として保存ボタンを追加します。
関連 Issue
スクリーンショット・動画など
(なし)
その他
(なし)