-
Notifications
You must be signed in to change notification settings - Fork 96
Refactoring / Bug fix #43
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
Conversation
Haruma-K
commented
May 5, 2025
- PageContainer, ModalContainer, SheetContainerをリファクタリング
- PushやPopなどの処理の流れを整理
- 複数の Page や Modal を同時に破棄した際に、終了処理が一部漏れることがあった問題を修正
- Refactored PageContainer, ModalContainer, and SheetContainer
- Streamlined the flow of operations such as Push and Pop
- Fixed an issue where certain termination processes might not be called when dismissing multiple Pages or Modals at once
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.
Pull Request Overview
This pull request refactors the lifecycle management for Sheets, Pages, and Modals while addressing a bug where termination processes could be partially skipped when dismissing multiple screens. Key changes include introducing dedicated context and lifecycle handler classes, consolidating transition logic using a shared ScreenContainerTransitionHandler, and updating the demo presenter to improve transition handling.
- Introduced context classes (e.g., SheetHideContext, PagePushContext, ModalPopContext) and corresponding lifecycle handlers.
- Refined transition flow by replacing exception throws with Assert-based checks and centralizing interaction state management.
- Fixed the issue of missed termination callbacks during simultaneous screen dismissals and added a wait frame in the demo presenter.
Reviewed Changes
Copilot reviewed 20 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Assets/UnityScreenNavigator/Runtime/Core/Sheet/SheetHideContext.cs | Added a context class for sheet hide transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Sheet/SheetContainer.cs | Refactored sheet registration and transition logic with integrated lifecycle and transition handling. |
| Assets/UnityScreenNavigator/Runtime/Core/Shared/ScreenContainerTransitionHandler.cs | Introduced a dedicated transition handler for managing container transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Shared/IScreenContainer.cs | Added an interface abstraction for screen containers. |
| Assets/UnityScreenNavigator/Runtime/Core/Page/PagePushContext.cs | Provided a push context structure for page transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Page/PagePopContext.cs | Provided a pop context structure for page transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Page/PageLifecycleHandler.cs | Updated page lifecycle operations to follow the new refactored patterns. |
| Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalPushContext.cs | Provided a push context structure for modal transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalPopContext.cs | Provided a pop context structure for modal transitions. |
| Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalLifecycleHandler.cs | Refactored modal lifecycle management including backdrop handling. |
| Assets/Demo/Core/Scripts/Presentation/Loading/LoadingPagePresenter.cs | Updated LoadingPagePresenter to introduce a one-frame wait before calling the home loading page shown callback. |
Files not reviewed (9)
- Assets/Demo/Core/DemoEntryPoint.unity: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalLifecycleHandler.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalPopContext.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Modal/ModalPushContext.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Page/PageLifecycleHandler.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Page/PagePopContext.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Page/PagePushContext.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Shared/IScreenContainer.cs.meta: Language not supported
- Assets/UnityScreenNavigator/Runtime/Core/Shared/ScreenContainerTransitionHandler.cs.meta: Language not supported
| { | ||
| view.StartCoroutine(WaitAndCallHomeLoadingPageShown()); | ||
| } | ||
|
|
Copilot
AI
May 5, 2025
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.
[nitpick] Consider adding a comment explaining why waiting one frame (via WaitAndCallHomeLoadingPageShown) is necessary before invoking the HomeLoadingPageShown callback.
| // This coroutine waits for one frame before invoking the HomeLoadingPageShown callback. | |
| // The delay ensures that any pending UI updates or state changes are completed | |
| // before the callback is triggered, preventing potential race conditions or errors. |