Fix pane focus rapidly switching back and forth

[osandov]

Switching panes multiple times too quickly sometimes causes the focus to get stuck rapidly switching back and forth between two panes by itself. This happens both locally and on an SSH domain. The root cause is similar in both cases.

In the local case, GuiFrontend handles a MuxNotification::PaneFocused event by calling Mux::focus_pane_and_containing_tab, which calls through Tab::set_active_pane -> TabInner::set_active_pane -> TabInner::advise_focus_change. TabInner::advise_focus_change then calls Mux::notify with a new MuxNotification::PaneFocused event, which is a redundant notification focusing the same pane again.

This is normally harmless other than a small amount of wasted work; TabInner::set_active_pane notices that the focused pane didn't change and doesn't generate yet another event.

However, if another real focus change is queued between the original focus change and the redundant one, we get into trouble. For example, if the user switches to pane 0 and then immediately to pane 1, the event queue contains [PaneFocused(0), PaneFocused(1)]. When the first event is handled, pane 0 is focused, and a redundant notification is generated, so the event queue contains [PaneFocused(1), PaneFocused(0)]. Then after the next event is handled, pane 1 is focused, and we add another redundant notification, so the event queue contains [PaneFocused(0), PaneFocused(1)]. Repeat ad nauseam.

Fix this by not generating the notification from
TabInner::advise_focus_change when it would be redundant, which we indicate with a new boolean parameter.

The mux server case is very similar, except that Pdu::SetFocusedPane in SessionHandler is the vector for the bug.

This bug appears to have been introduced by commit f71bce1. I verified that wezterm cli activate-pane-direction still works over SSH after this fix.

closes: #4390
closes: #4693

[wez]

Thanks for this! I've only quickly skimmed this without really following the call stacks through at the moment, and have one quick and easy bit of feedback!

[osandov]

Done. I went with NotifyMux as the name, although I'm happy to change it to anything.

[wez]

Thanks! Now, do wezterm cli activate-tab, wezterm cli activate-pane-direction and wezterm cli activate-pane still work after this change?

[osandov]

Ugh, I tested wezterm cli activate-pane-direction and it worked, but the other two no longer work over SSH :( they all still work locally. I also just tested having multiple clients attached to the same SSH session, and this also broke syncing the pane focus updates between those.

My guess is that we need to avoid sending a redundant update back to the source it originally came from, but we need to send updates everywhere else. Is that possible?

P.S. There are some vaguely related issues I've run into with SSH sessions that I hadn't written up as issues yet: rotating panes through the command palette gets undone on the next keypress, and resizing panes by dragging the borders flickers. Ignoring Pdu::TabResized events in the client "fixes" those for me, but it breaks syncing clients, too. So I wonder if there's a more general solution here.

[wez]

Thanks for checking!

I'm not in the right frame of mind to seriously design this right now, but I'll throw out a couple of possible approaches and let you noodle on them.

1. We know who the client is and could pass that through as part of the NotifyMux enum you added. Then it could get filtered out from being sent back to the originating client. Not sure if I like this idea because it might break some subtle wakeup somewhere.
2. We have an InputSerial concept that likely could be applied here. I honestly can't remember the details of how it works without looking through the code, but look for InputSerial and input_serial to get the gist of it. There could be some filtering applied by the client that will discard focus notifications if the input serial is older than some checkpoint that it has established already. I don't have a good suggestion for how to frame that checkpoint off the top, but here's where the serial is used today to avoid other unpleasantness:

wezterm/wezterm-client/src/pane/renderable.rs

Lines 333 to 346 in 4921f13

	// When it comes to updating the cursor position, if the update was tagged
	// with keyboard input, we'll only take the position if the update comes from
	// the most recent key event. This helps to prevent the cursor wiggling if the
	// user is typing more than one character per roundtrip interval--the wiggle
	// manifests because we may have already predicted a local cursor move forwards
	// by one character, and we may receive the response to the prior update after
	// we have rendered that, and then shortly receive the most recent response.
	// The result of that is that the cursor moves right one, left one and then
	// finally right one in quick succession.
	// If the delta was not from an input event then we trust it; this is most
	// like due to a unilateral movement by the application on the other end.
	if delta.input_serial.is_none()
	|| delta.input_serial.unwrap_or(InputSerial::empty()) >= self.input_serial
	{

[crides]

Tested. This does fix the problem when it's caused by the terminal hanging on much output (i.e. #4517).

[osandov]

I'm not going to get around to resolving the other muxing issues here, so I'm going to close this. Anyone else is welcome to pick this up.