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.

Sign up for GitHub

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

Jump to bottom

[project-sequencer-statemachine] transitionToでonExitが呼ばれた場合でもステートが正しく動作するようにする #2519

Merged
merged 2 commits into from
Feb 4, 2025
Merged

[project-sequencer-statemachine] transitionToでonExitが呼ばれた場合でもステートが正しく動作するようにする #2519

merged 2 commits into from
Feb 4, 2025
+162 −99

Conversation

sigprogramming
Copy link
Contributor

@sigprogramming sigprogramming commented Feb 3, 2025 •
edited
Loading

内容

#2518 でtransitionToメソッドを追加したので、（processのsetNextStateではなく）transitionToでonExitが呼ばれることも考慮する必要があります。
これを考慮して、transitionToでonExitが呼ばれた場合（processが一度も呼ばれずにonExitが呼ばれた場合も含む）でもステートが正しく動作するようにします。
（mouseupの前にonExitが呼ばれた場合はプレビューの適用を行わないようにします）

また、以下も行います。

  • onEnterでもguideLineTicksの更新を行うようにする
  • SelectNotesWithRectStateでcontext.nowPreviewingの更新を行うようにする

関連 Issue

その他

@sigprogramming sigprogramming requested a review from a team as a code owner February 3, 2025 14:58
@sigprogramming sigprogramming requested review from Hiroshiba and removed request for a team February 3, 2025 14:58
@sigprogramming sigprogramming changed the title [project-sequencer-statemachine] onEnterの直後にonExitが呼ばれた場合でもステートが正しく動作するようにする [project-sequencer-statemachine] transitionToでonExitが呼ばれた場合でもステートが正しく動作するようにする Feb 3, 2025
Hiroshiba
Hiroshiba reviewed Feb 3, 2025
View reviewed changes
Copy link
Member

@Hiroshiba Hiroshiba left a comment •
edited
Loading

Choose a reason for hiding this comment

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

おそらくLGTMなのですが、ちょっと前提の確認だけ！

transitionToの実装目的って何だった感じでしょうか 👀
僕の理解なのですが、マウスクリックやショートカットキーでツール選択したときにこのtransitionToを直接呼ぶ感じを想定している、というのであってそうでしょうか！

（他の前提の候補として、テスト用とかに書いたとかもありえるなぁと。であれば、正常系のテストはマウスイベント等を渡しまくれば OK なので、異常系のテスト用なはずで、であればレビューの対象が変わりそうだなーと。）

src/sing/sequencerStateMachine/states/moveNoteState.ts Outdated Show resolved Hide resolved
src/sing/sequencerStateMachine/states/resizeNoteLeftState.ts Outdated Show resolved Hide resolved
@sigprogramming
Copy link
Contributor Author

sigprogramming commented Feb 4, 2025 •
edited
Loading

transitionToの実装目的って何だった感じでしょうか 👀
僕の理解なのですが、マウスクリックやショートカットキーでツール選択したときにこのtransitionToを直接呼ぶ感じを想定している、というのであってそうでしょうか！

あってます！ツール選択でtransitionToを呼ぶのを考えています。
外から強制的に遷移させる形であまり良くないので、遷移の条件を定義して遷移する形の設計に変更した方が良いかもとちょっと思っていますが、今はこだわらずに一通り実装した方が良い気がするので、一旦これで…！

@Hiroshiba
Copy link
Member

あってます！ツール選択でtransitionToを呼ぶのを考えています。
外から強制的に遷移させる形であまり良くないので、遷移の条件を定義して遷移する形の設計に変更した方が良いかもとちょっと思っていますが、今はこだわらずに一通り実装した方が良い気がするので、一旦これで…！

なるほどです、方針としては良さそうに思いました！
あまりtransitionToを積極的に呼んで良い設計にはなっていない、ということをドキュメントで案内した方が良いかもですね！
こっちは思いつきですが、Idle状態に対してしかtransitionToできないようにしてもいいかも。

Hiroshiba
Hiroshiba approved these changes Feb 4, 2025
View reviewed changes
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！！

transitionToのドキュメント追加はいつかやっておいた方が良さそうかなと思いました！

@Hiroshiba Hiroshiba merged commit 036029a into VOICEVOX:project-sequencer-statemachine Feb 4, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hiroshiba Hiroshiba Hiroshiba approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sigprogramming @Hiroshiba