Making sure kitty setup runs after terminal setup

[cxreiff]

Changes

• added a rule for kitty::setup to run after terminal::setup.

Notes

It looks like moving the kitty protocol setup into its own plugin broke keyboard input- sometimes protocol setup runs before terminal setup which borks the terminal. While I believe its also possible for the cleanup functions to run out-of-order as well, I didn't notice any issues testing in a few terminals so I think we can leave it alone.

Testing

Tested with Alacritty, iTerm, Kitty on macOS.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (89e618b) to head (9eecfff).

Files	Patch %	Lines
src/terminal.rs	0.00%	9 Missing ⚠️
src/kitty.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main      #6   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          6       6           
  Lines        165     173    +8     
=====================================
- Misses       165     173    +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cxreiff]

Actually, hold on this a moment. While the terminal appears normal after cleanup, opening neovim and typing certain characters gets escaped differently, so there is something left over. I think I need to re-add the protocol cleanup to be in line with the ratatui cleanup.

[joshka]

That's actually two perfect reasons to reopen the conversations about teardown systems:

• Teardown/exit systems bevyengine/bevy#7067
• Exit schedule for systems to run at end of app run bevyengine/bevy#7355

We need here:

• guaranteed order of teardown systems
• the ability to call the teardown systems from other systems

[joshka]

Side note - if we moved some of the calls into a response to AppExit, do you think it's possible to test these somehow, or is the non-determinism in order expected to be always a problem there?

I'd happily merge the startup changes - they make sense. I'm not sure about the shutdown changes.

BTW, I was thinking about possibly doing the same split for alternate screen / raw mode as I did for kitty / mouse.

Any particular idea which call is the one that makes the kitty stuff break if it comes after.

[joshka]

I wonder if there's a way to (deterministically) get this to occur before the other things that are dropped.

Perhaps by calling remove_resource in response to AppExit? Does doing that allow us to order the systems perhaps?

I think we might be able to handle this kind of the same way that a panic hook in normal ratatui does - treat the drop as the last ditch effort to cleanup, but to think of the app actively cleaning up when it can.

[cxreiff]

Side note - if we moved some of the calls into a response to AppExit, do you think it's possible to test these somehow, or is the non-determinism in order expected to be always a problem there?

I don't quite understand, but I think what you are reaching for is using an exclusive system with World access and calling remove_resource there. In that case the resource is removed immediately as opposed to batched for execution later like usual. You could theoretically do this for each resource that has an order-critical Drop implementation but... at this point you'd have strayed pretty far from typical idiomatic bevy and would be adding a lot of boilerplate and layers of indirection for the benefit of separating these different parts of configuring the terminal environment.

I agree that it's a little more "Rust-y" to decouple by having each struct be responsible for cleaning itself up, but... I would also argue that these separate pieces of cleanup are not actually that decoupled given that the wrong order can cause problems. If the order of execution is critical, a synchronous block of code is hard to get wrong, and a plugin whose responsibility is configuring the terminal environment and restoring it afterwards is not too overburdened in my opinion.

Any particular idea which call is the one that makes the kitty stuff break if it comes after.

The specific issues I ran into (random escape sequences in the prompt instead of characters, doubly-typed characters, issues in neovim) were caused by PopKeyboardEnhancementFlags being applied after LeaveAlternateScreen. I changed the mouse cleanup as well because I suspected that the flag would fail to be removed in the same way, but I have not confirmed that for sure.

[cxreiff]

I tried this and got the same issues with the keyboard enhancement not being cleaned up:

app.add_systems(PostUpdate, cleanup_system);

pub fn cleanup_system(mut commands: Commands, mut exit_reader: EventReader<AppExit>) {
    for _ in exit_reader.read() {
        commands.remove_resource::<KittyEnabled>();
        commands.remove_resource::<MouseCaptureEnabled>();
        commands.remove_resource::<RatatuiContext>();
    }
}

Commands should execute in the order they are added, but given that this isn't working, I need to dig a little deeper.

[cxreiff]

Clarification, issues only when exiting with a panic, in this case. So not an order issue, the panic is preventing the Bevy systems from firing, so the only way of recovering in this case is the panic hook. At that point we have left the world of bevy so... we don't really have an easy way of retrieving the resource in order to call drop() on it or whatever else. The only reason this commit was working was because the panic hook manually calls restore(), its actually not going through the Drop implementation before the alternate view exits, as far as I can tell.

[cxreiff]

I've reverted the cleanup changes for now and added the aforementioned cleanup_system instead that will ensure the correct cleanup order at least in the case of a normal AppExit event (still skipped by a panic).

Would you be amenable to merging this to at least get this package un-broken, and cleaning up the terminal after a panic can be handled separately?

[joshka]

I've reverted the cleanup changes for now and added the aforementioned cleanup_system instead that will ensure the correct cleanup order at least in the case of a normal AppExit event (still skipped by a panic).

Would you be amenable to merging this to at least get this package un-broken, and cleaning up the terminal after a panic can be handled separately?

Yup - thanks for the PR :)

One think I noticed when experimenting was that the order of running drops after removing the resources with a command seemed to be the same order as the items were created, regardless of the order that they were removed. This seems opposite to rust's normal drop order (which is reverse order of creation in many forms). My experiment was just using a couple of synthetic resources to work out what the behavior was, so I'm not 100% certain that this is something that applies generically.

[cxreiff]

Thanks! The thing I'm still confused over is the Drop impls not being called, my understanding was that Rust always "unwinds" after a panic wherein it calls 'drop' on every remaining struct. I can only assume that the logging that I'm using to test is getting swallowed by the alternate view in some way I haven't yet been able to pick apart.

[joshka]

I can confirm that (even with simplified testing), panicking seems to swallow the drops, that's annoying... :/