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

[ソング] Undo/Redoの実装 #1836

Merged
merged 36 commits into from
Feb 20, 2024

Conversation

y-chan
Copy link
Member

@y-chan y-chan commented Feb 9, 2024

内容

題の通りです。ソングエディタにundo/redoをボタンで実装します。
ソングエディタのundo/redoキューはトークと独立しています。
ショートカットキーについては元の状態からいじっていないため、このPRだけでIssueの解決は完結しません。

また、undo/redoの達成のために、一部の処理を変更しました。
具体的には、OverlappingNotesDetectorです。
この中にはstoreから独立した、ノートの重なり情報が保存されており、それがundo/redoによって同期しなくなる問題があるため、それらの情報をstore側に移動し、OverlappingNotesDetectorに挿入するようにすることで、storeと同期できるようにします。

関連 Issue

ref #1834

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

image

その他

現状のsingingCommandStoresingingStoreのほぼコピペであり、メンテナンス性が悪いので一旦Draftです。
私は #1829 にも取り組んでいるので、もし取り組める方がいらっしゃれば、このPRをベースに新しく組んでいただいてもOKです。

@Hiroshiba
Copy link
Member

Hiroshiba commented Feb 11, 2024

PRありです!!

こちら取り組まれているか、取り組むつもりかがわからず他の方が手をつけづらい感じになっちゃってるかもです。
安心して他の方も取り組めるように再度取り組み始めたらコメント等で合図を頂くというのはどうでしょう🙇

@y-chan
Copy link
Member Author

y-chan commented Feb 12, 2024

一旦手を止めているので、お任せできる方がいれば、宣言していただければこの後のリファクタリングをお任せしたいといったつもりでした...言葉足らずですみません 🙇

ひとまずプロジェクト保存のPRの方に専念したいと思うので、 #1829 がマージされるまでに、取りくまれる方がいらっしゃらなければ、こちらにコメントを残したうえで引き続き私の方でこのPRを完成させたいと思います....!

@y-chan
Copy link
Member Author

y-chan commented Feb 18, 2024

#1829 がマージされたのでこちら取り組み始めます!

@y-chan y-chan marked this pull request as ready for review February 18, 2024 05:43
@y-chan y-chan requested a review from a team as a code owner February 18, 2024 05:43
@y-chan y-chan requested review from Hiroshiba and removed request for a team February 18, 2024 05:43
@y-chan
Copy link
Member Author

y-chan commented Feb 18, 2024

不要と思われるstore actionの削除や、処理の統合、コンフリクトの解消を行い、ready for reviewにしました!

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.

色々コメントしていますけれども方針としては問題ないと思います!!
多分ちょこちょこリファクタリングしさえすればすぐマージできるかなと!!

変更お疲れ様でした!!!

src/components/Sing/ToolBar.vue Outdated Show resolved Hide resolved
src/store/type.ts Outdated Show resolved Hide resolved
src/sing/storeHelper.ts Outdated Show resolved Hide resolved
src/sing/storeHelper.ts Outdated Show resolved Hide resolved
src/store/command.ts Outdated Show resolved Hide resolved
src/store/command.ts Outdated Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
src/store/singing.ts Show resolved Hide resolved
@y-chan
Copy link
Member Author

y-chan commented Feb 19, 2024

@Hiroshiba @Segu-g
レビューありがとうございます!

総合的に見て、storeは

undo(redo)Commands: {
  talk: Command[],
  song: Command[],
}

の形が良いと思いました!
この方が一般的にできて拡張性も高いと思うのですが、気がつけませんでした...ありがとうございます...!

その他、レビューいただいたコメントのコピー忘れやクラスの解体、リファクタ、それらに加えてEditorTypeを作って活用するようにしました(他でもtalk | songな型が存在したので、統合しました)

再レビューお願いします...!

@y-chan y-chan requested review from Hiroshiba and Segu-g February 19, 2024 03:00
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.

良い感じだと思います!!

src/sing/storeHelper.ts Outdated Show resolved Hide resolved
src/store/command.ts Outdated Show resolved Hide resolved
y-chan and others added 2 commits February 19, 2024 12:17
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
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!!!

動作チェックも行いました、お疲れ様でした!!!
ちょっとこっちで UI 微調整させていただきます!
(undo/redoを右側に持ってくるなど)


@romot-co

ボタンの順番ですが、こんな感じで大丈夫でしょうか?
undo/redoを一番右に持ってくるべきかをちょっと迷ってます。
image

@Hiroshiba
Copy link
Member

とりあえず一旦問題ないと思うのでマージします!!

@Hiroshiba Hiroshiba merged commit 008f020 into VOICEVOX:main Feb 20, 2024
9 checks passed
@romot-co
Copy link
Contributor

romot-co commented Feb 21, 2024

@Hiroshiba
こちら現在の位置(編集系ツールがある部分の左)がよいかと思います!

  • 今後、使いづらいという意見が多数出たら編集系ツールの位置を考慮する
  • そろそろメニューに「編集」が必要そう(まずはundo/redoのみ)

以下の「編集」あたり...Issue立てても問題なさそうであれば、立てます!
https://docs.google.com/spreadsheets/d/1ltGxtvCoUhDiX06F8E1-Tf1TWlC9XM98ESBNOIHUYjw/edit#gid=1397773732

@Hiroshiba
Copy link
Member

@romot-co ありがとうございます!!

そろそろメニューに「編集」が必要そう(まずはundo/redoのみ)
以下の「編集」あたり...Issue立てても問題なさそうであれば、立てます!

issue立てて問題ないと思います!

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.

4 participants