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

#1877 ソング: 編集メニューの追加とノートのコピー&ペーストの実装 #1903

Merged
merged 19 commits into from
Mar 22, 2024

Conversation

romot-co
Copy link
Contributor

@romot-co romot-co commented Mar 4, 2024

内容

機能追加

  • ノートのコピー/切り取り/貼り付け機能の実装およびキーバインド
  • ノートの全選択の実装

メニューの追加と変更

  • メインメニュー: 編集メニューの追加
    トーク: 元に戻す/やり直す
    ソング: 元に戻す/やり直す/コピー/切り取り/貼り付け/すべて選択/選択解除

  • シーケンサ右クリックメニューの追加
    選択中を削除/コピー/切り取り/貼り付け/すべて選択/選択解除

  • ノート右クリックメニューの調整
    コピー/切り取りを追加

関連 Issue

ref #1877
close #1877

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

スクリーンショット 2024-03-05 8 17 08

その他

そこまで難易度高そうではなかったため、コピペもあわせて実装いたしました。

@romot-co romot-co requested a review from a team as a code owner March 4, 2024 23:27
@romot-co romot-co requested review from Hiroshiba and removed request for a team March 4, 2024 23:27
@romot-co romot-co self-assigned this Mar 4, 2024
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/store/singing.tsのコードは @sigprogramming さんにもレビューいただけると心強いかもです!

src/backend/electron/main.ts Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/components/Sing/ScoreSequencer.vue Outdated Show resolved Hide resolved
src/components/Sing/ToolBar.vue Outdated Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
Comment on lines 2012 to 2013
// クリップボードのテキストをJSONとしてパースする(失敗した場合は何もしない)
const notesJson = JSON.parse(text);
Copy link
Member

Choose a reason for hiding this comment

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

ここはzodを使うと型サポート受けられるようにも出来たりします!(noteSchemaのimportが必要)

Suggested change
// クリップボードのテキストをJSONとしてパースする(失敗した場合は何もしない)
const notesJson = JSON.parse(text);
// クリップボードのテキストをJSONとしてパースする(失敗した場合は何もしない)
const notes = noteSchema.array().parse(JSON.parse(text));

copy元でidを除外してる場合はこんな感じかなと!

Suggested change
// クリップボードのテキストをJSONとしてパースする(失敗した場合は何もしない)
const notesJson = JSON.parse(text);
// クリップボードのテキストをJSONとしてパースする(失敗した場合は何もしない)
const notes = noteSchema
.omit({ id: true })
.array()
.parse(JSON.parse(text));

Comment on lines 2025 to 2036
const notesToPaste: Note[] = notesJson.map((note) => {
// 新しい位置を現在の再生位置に合わせて計算する
const pastePosition =
Number(note.position) - firstNotePosition + currentPlayheadPosition;
return {
id: uuidv4(),
position: Number(pastePosition),
duration: Number(note.duration),
noteNumber: Number(note.noteNumber),
lyric: String(note.lyric),
};
});
Copy link
Member

Choose a reason for hiding this comment

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

お行儀悪いかもですが破壊的変更の形にすると綺麗かも。

Suggested change
const notesToPaste: Note[] = notesJson.map((note) => {
// 新しい位置を現在の再生位置に合わせて計算する
const pastePosition =
Number(note.position) - firstNotePosition + currentPlayheadPosition;
return {
id: uuidv4(),
position: Number(pastePosition),
duration: Number(note.duration),
noteNumber: Number(note.noteNumber),
lyric: String(note.lyric),
};
});
notes.forEach((note) => {
// 新しい位置を現在の再生位置に合わせて計算する
note.position =
note.position - firstNotePosition + currentPlayheadPosition;
note.id = uuidv4();
});

src/type/preload.ts Show resolved Hide resolved
src/components/Sing/SequencerNote.vue Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
@Hiroshiba
Copy link
Member

Hiroshiba commented Mar 5, 2024

あ、あとスクリーンショットテストが落ちてるので更新していただければ!
[update snapshots]という文字列を含むコミットメッセージを入れてpushすると、Github Actionsが自動的に更新して追加pushしてくれます。

詳しくはこちら

romot-co and others added 4 commits March 7, 2024 00:34
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
Copy link
Contributor

@sigprogramming sigprogramming left a comment

Choose a reason for hiding this comment

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

コピー&ペーストの実装ありがとうございます!!

src/store/singing.ts Outdated Show resolved Hide resolved
@romot-co
Copy link
Contributor Author

romot-co commented Mar 9, 2024

いったん指摘点修正済み・クオンタイズ追加(一応動いているように見える)

src/store/singing.ts Outdated Show resolved Hide resolved
src/store/singing.ts Outdated Show resolved Hide resolved
Comment on lines 829 to 832
// OSXの場合、Ctrl+クリックが右クリックのため、その場合はノートを追加しない
if (event.ctrlKey && event.button === 0) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

なるほどです!!
ここにisMacがあるのでそれも加えるといいかも?

src/store/singing.ts Show resolved Hide resolved
@Hiroshiba
Copy link
Member

あっ スクショテストがまた落ちてますね。。
mainブランチをマージした後にスクショ更新が必要だったかもです 🙇
#1903 (comment)

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 172 to 191
{
type: "button",
label: "元に戻す",
onClick: () => {
if (uiLocked.value) return;
store.dispatch("UNDO", { editor });
},
disableWhenUiLocked: true,
disabled: !canUndo.value,
},
{
type: "button",
label: "やり直す",
onClick: () => {
if (uiLocked.value) return;
store.dispatch("REDO", { editor });
},
disableWhenUiLocked: true,
disabled: !canRedo.value,
},
Copy link
Member

Choose a reason for hiding this comment

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

この2つはsingとtalkで同じなので統一できそうですね!!
実際に「プロジェクト上書き保存」とかはBaseMenuBar内で統一されてるので、書き方を真似れば統一できると思います!

{
type: "button",
label: "プロジェクトを上書き保存",
onClick: async () => {
await saveProject();
},
disableWhenUiLocked: true,
},
{

分からなかったら後でプルリクエスト出させていただきます!

src/store/singing.ts Show resolved Hide resolved
src/type/preload.ts Show resolved Hide resolved
@romot-co
Copy link
Contributor Author

いったん指摘点修正済み

@Hiroshiba
Copy link
Member

おっとテストが回ってないかもですね! VOICEVOXリポジトリへの自動pushだからかな、謎です。
draft化して戻すと確かテストまわるので、ちょっと一旦draft化させていただきます! 🙇

@Hiroshiba Hiroshiba marked this pull request as draft March 20, 2024 22:58
@Hiroshiba Hiroshiba marked this pull request as ready for review March 20, 2024 22:58
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/components/Menu/MenuBar/BaseMenuBar.vue Show resolved Hide resolved
return;
}

// TODO: メニューが表示されている場合はメニュー非表示のみ行いたい
Copy link
Member

Choose a reason for hiding this comment

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

ちょっと考えたのですが同意見です!

よくある挙動だとコンテキストメニュー開いた後のクリックも普通のクリックと同じ扱いにする(ボタンを押すなど)のですが、どこをクリックしても何かが発動するシーケンサーの場合は話は別なのかなぁと思いました。

{ type: "separator" },
{
type: "button",
label: "クオンタイズ",
Copy link
Member

Choose a reason for hiding this comment

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

ここの名称、初心者にはかなり難しそうですね。。。。
まあこれを必要とする人はわりと玄人だと思うので大丈夫そうではありますが・・・

ちょっと初心者向けのワードを考えてみました。
やることは「ノートをリズムに合わせる」ですが、それだと長いので、「ノート整列」とか「整列」とか「タイミング合わせ」とか「タイミング調整」とかでしょうか。
長いけど「タイミング調整(クオンタイズ)」とか・・・・?

うーん、とりあえずクオンタイズのままで!

Comment on lines 2028 to 2029
dispatch("COPY_NOTES_TO_CLIPBOARD");
dispatch("COMMAND_REMOVE_SELECTED_NOTES");
Copy link
Member

Choose a reason for hiding this comment

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

一応await挟んでおくと安心かも?

src/store/singing.ts Outdated Show resolved Hide resolved
@Hiroshiba Hiroshiba merged commit 7096c46 into main Mar 22, 2024
18 checks passed
@Hiroshiba Hiroshiba deleted the feature/1897_add_edit_menu_and_copy_paste branch March 22, 2024 10:54
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.

None yet

3 participants