Consistently close the terminal - #58
Conversation
Signed-off-by: HigherOrderLogic <73709188+HigherOrderLogic@users.noreply.github.com>
cb778c8 to
4f3615e
Compare
ernestrc
left a comment
ernestrc
left a comment
There was a problem hiding this comment.
Thanks for taking the initiative to fix this.
In principle, I'm not opposed to broadcasting term.EventNone from the workspace manager to all workspaces, and from each browser to all of its windows and free tabs. That does seem appealing, but I'm not convinced by the exitReporter interface. I think it overcomplicates the logic and isn't future-proof: a single intermediate wrapper that doesn't implement Exited breaks the chain of interface type assertions. The lack of tests makes this even more concerning.
There are also cases this doesn't address, such as extension-provided tui.Handler implementations installed via RPC, which use handlerrpc.ClientStream[T] and have the same known limitation. Maybe the approach of sending term.EventNone to force a call to .Handle, thereby prompting collections to remove exiting children, has run its course.
The vte already knows when the shell exits (where we set e.exit in vte.Handler), and it already talks outward to the browser: browser.TabManager is passed into vte.NewHandler. We can declare a new interface in the vte package with just the method vte needs, and browser.Component satisfies it directly which it's already what gets passed as TabManager today.
// declared in package vte
type TabExiter interface {
OnTabExit(uri workspaceapi.URI) bool
}That gives us a compile-checked contract instead of an assertion chain, identity comes from the URI at call time, and it works the same for free tabs and for windows in non-focused workspaces; workspaceTabManager.SetTabName already hops scheduleNextTick and updates a workspace that isn't in focus.
Also, please always add tests that cover, at a minimum, the bug we're trying to fix. Otherwise, we risk reintroducing the issue in the future.
| } | ||
| exit, handled = c.union.Handle(ev) | ||
| if exit { | ||
| c.RemoveWindowContent(c.focus()) |
There was a problem hiding this comment.
c.focus() is read after c.union.Handle(ev), so it is not necessarily the window that exited.
| } | ||
|
|
||
| func (av *asyncVTE) Exited() bool { | ||
| if e, ok := av.real.(interface{ Exited() bool }); ok { |
There was a problem hiding this comment.
clever, but not necessary. An anonymous interface type assertion should be a last resort.
| return c.union.Handle(ev) | ||
| if ev.Type == term.EventNone { | ||
| focusID := c.wm.Focus().ID() | ||
| for _, t := range slices.Clone(c.buffers) { |
There was a problem hiding this comment.
No need to clone the slice here. It's causing allocations for no real benefit.
| } | ||
| }) | ||
| for _, id := range closing { | ||
| if bw, ok := c.findWindow(id); ok { |
There was a problem hiding this comment.
We already had the window, why not store a slice of *browserWindow, so we don't need to do a map lookup here?
| focusID := c.wm.Focus().ID() | ||
| for _, t := range slices.Clone(c.buffers) { | ||
| if t.free && t.Exited() { | ||
| t.Handle(ev) |
There was a problem hiding this comment.
browser.Tab.Handle should be called when an event is dispatched to a particular tui.Handler.
This does what you intend it to do (*browser.Tab calling its parent browser and removing itself when it exits) but for a future reader of this code it might be quite hard to understand at first. I get that it's correct today, but we should strive to write code that it's easy to read for someone else in the future, no implicit contracts. In this same function, we're removing windows by calling *browserWindow.Close, removing tabs by calling *browser.Tab.Handle and removing tabs/content from a window again via *browser.Component.RemoveWindowContent.
2 participants
Previously, running
exitin a terminal only closed it when it's the focused handler. This patches always close the terminal when it exits.