Conversation
4a3a7e0 to
a0df056
Compare
|
Focus after destruction is off, and cursor focusing reports errors in the log, but this is good enough for now. |
There was a problem hiding this comment.
Pull request overview
This PR implements proper shutdown and restart functionality for instances, enabling them to gracefully terminate in response to CMD+w and restart when requested. The changes focus on adding shutdown coordination, proper cleanup of resources including scene change collectors, and updating the presentation layer to support hiding instances and views.
Changes:
- Renamed methods for clarity:
upgrade_to_apply_animations→upgrade_to_apply_animations_cycleandchange_tracker→change_collector - Added shutdown handling with resource cleanup:
DestroyViewnow includesChangeCollectorfor proper cleanup validation - Implemented
hide_instanceandhide_viewmethods throughout the presentation layer to support graceful teardown - Added error handling and logging for instance lifecycle events
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shell/src/application_context.rs | Updated method name to upgrade_to_apply_animations_cycle |
| scene/src/scene.rs | Renamed change_tracker to change_collector and added into_collector method |
| scene/src/change_collector.rs | Added Drop implementation to detect unprocessed changes and memory optimization in accumulate/release |
| scene/Cargo.toml | Added log dependency for error reporting |
| desktop/src/projects/project_presenter.rs | Added hide_instance and hide_view methods for graceful instance/view teardown |
| desktop/src/projects/launcher_presenter.rs | Moved apply_animations method and added hide_instance/hide_view implementations |
| desktop/src/instance_presenter.rs | Renamed state Appearing to WaitingForPrimaryView and removed view from Disappearing state |
| desktop/src/instance_manager.rs | Renamed stop to trigger_shutdown and removed unused helper methods |
| desktop/src/desktop_presenter.rs | Added hide_instance and updated hide_view to route through appropriate presenter |
| desktop/src/desktop_interaction.rs | Added architecture comment about view termination |
| desktop/src/desktop.rs | Implemented shutdown workflow with focus management, error handling, and collector reference validation |
| desktop/src/band_presenter.rs | Implemented hide_instance and hide_view with proper state transitions |
| applications/src/view_builder.rs | Reordered fields and added scene parameter to enable View ownership of Scene |
| applications/src/view.rs | Made View own Scene, updated Drop to send collector, and added render and scene methods |
| applications/src/scene.rs | Added into_collector method to expose underlying change collector |
| applications/src/instance_context.rs | Updated DestroyView command to include ChangeCollector and method name to upgrade_to_apply_animations_cycle |
| animation/src/coordinator.rs | Renamed upgrade_to_apply_animations to upgrade_to_apply_animations_cycle |
|
|
||
| role: ViewRole, | ||
|
|
There was a problem hiding this comment.
Fields are grouped with a blank line separating 'scene' from 'role', creating an inconsistent grouping pattern. Consider either removing the blank line between 'scene' and 'role' to keep all fields together, or adding a comment to explain why these fields are logically separated.
| role: ViewRole, | |
| role: ViewRole, |
| pub fn build(self) -> Result<View> { | ||
| View::new( | ||
| self.instance, | ||
| self.command_sender, | ||
| self.role, | ||
| self.instance, | ||
| self.extent, | ||
| scene, | ||
| self.scene, | ||
| self.role, | ||
| ) | ||
| } |
There was a problem hiding this comment.
The argument order in View::new (command_sender, instance, extent, scene, role) differs from the field declaration order in ViewBuilder. Consider reordering the arguments to match the struct field order for consistency and maintainability.
This PRs goal is to to properly shutdown instances in response to CMD+w and enable them to restart in response to a user request (currently either by clicking a project launcher or CMD+t in the top band).