Skip to content

Commit

Permalink
Fix pane focus rapidly switching back and forth
Browse files Browse the repository at this point in the history
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
  • Loading branch information
osandov committed Jan 2, 2024
1 parent ff27437 commit 2f7ca41
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 17 deletions.
3 changes: 2 additions & 1 deletion lua-api-crates/mux/src/pane.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::*;
use luahelper::{dynamic_to_lua_value, from_lua, to_lua};
use mlua::Value;
use mux::tab::NotifyMux;
use std::cmp::Ordering;
use std::sync::Arc;
use termwiz::cell::SemanticType;
Expand Down Expand Up @@ -400,7 +401,7 @@ impl UserData for MuxPane {
let tab = mux
.get_tab(tab_id)
.ok_or_else(|| mlua::Error::external(format!("tab {tab_id} not found")))?;
tab.set_active_pane(&pane);
tab.set_active_pane(&pane, NotifyMux::Yes);
Ok(())
});

Expand Down
4 changes: 2 additions & 2 deletions mux/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::client::{ClientId, ClientInfo};
use crate::pane::{Pane, PaneId};
use crate::tab::{SplitRequest, Tab, TabId};
use crate::tab::{NotifyMux, SplitRequest, Tab, TabId};
use crate::window::{Window, WindowId};
use anyhow::{anyhow, Context, Error};
use config::keyassignment::SpawnTabDomain;
Expand Down Expand Up @@ -542,7 +542,7 @@ impl Mux {
.get_tab(tab_id)
.ok_or_else(|| anyhow::anyhow!("tab {tab_id} not found"))?;

tab.set_active_pane(&pane);
tab.set_active_pane(&pane, NotifyMux::No);

Ok(())
}
Expand Down
28 changes: 19 additions & 9 deletions mux/src/tab.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ pub type Cursor = bintree::Cursor<Arc<dyn Pane>, SplitDirectionAndSize>;
static TAB_ID: ::std::sync::atomic::AtomicUsize = ::std::sync::atomic::AtomicUsize::new(0);
pub type TabId = usize;

#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub enum NotifyMux {
Yes,
No,
}

#[derive(Default)]
struct Recency {
count: usize,
Expand Down Expand Up @@ -698,8 +704,8 @@ impl Tab {
self.inner.lock().get_active_idx()
}

pub fn set_active_pane(&self, pane: &Arc<dyn Pane>) {
self.inner.lock().set_active_pane(pane)
pub fn set_active_pane(&self, pane: &Arc<dyn Pane>, notify: NotifyMux) {
self.inner.lock().set_active_pane(pane, notify)
}

pub fn set_active_idx(&self, pane_index: usize) {
Expand Down Expand Up @@ -1747,7 +1753,7 @@ impl TabInner {
self.active
}

fn set_active_pane(&mut self, pane: &Arc<dyn Pane>) {
fn set_active_pane(&mut self, pane: &Arc<dyn Pane>, notify: NotifyMux) {
if let Some(item) = self
.iter_panes_ignoring_zoom()
.iter()
Expand All @@ -1756,22 +1762,26 @@ impl TabInner {
let prior = self.get_active_pane();
self.active = item.index;
self.recency.tag(item.index);
self.advise_focus_change(prior);
self.advise_focus_change(prior, notify);
}
}

fn advise_focus_change(&mut self, prior: Option<Arc<dyn Pane>>) {
fn advise_focus_change(&mut self, prior: Option<Arc<dyn Pane>>, notify: NotifyMux) {
let mux = Mux::get();
let current = self.get_active_pane();
match (prior, current) {
(Some(prior), Some(current)) if prior.pane_id() != current.pane_id() => {
prior.focus_changed(false);
current.focus_changed(true);
mux.notify(MuxNotification::PaneFocused(current.pane_id()));
if notify == NotifyMux::Yes {
mux.notify(MuxNotification::PaneFocused(current.pane_id()));
}
}
(None, Some(current)) => {
current.focus_changed(true);
mux.notify(MuxNotification::PaneFocused(current.pane_id()));
if notify == NotifyMux::Yes {
mux.notify(MuxNotification::PaneFocused(current.pane_id()));
}
}
(Some(prior), None) => {
prior.focus_changed(false);
Expand All @@ -1786,7 +1796,7 @@ impl TabInner {
let prior = self.get_active_pane();
self.active = pane_index;
self.recency.tag(pane_index);
self.advise_focus_change(prior);
self.advise_focus_change(prior, NotifyMux::Yes);
}

fn assign_pane(&mut self, pane: &Arc<dyn Pane>) {
Expand Down Expand Up @@ -1849,7 +1859,7 @@ impl TabInner {
if keep_focus {
self.set_active_idx(pane_index);
} else {
self.advise_focus_change(Some(pane));
self.advise_focus_change(Some(pane), NotifyMux::Yes);
}
None
}
Expand Down
9 changes: 4 additions & 5 deletions wezterm-mux-server-impl/src/sessionhandler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use mux::client::ClientId;
use mux::domain::SplitSource;
use mux::pane::{Pane, PaneId};
use mux::renderable::{RenderableDimensions, StableCursorPosition};
use mux::tab::TabId;
use mux::tab::{NotifyMux, TabId};
use mux::{Mux, MuxNotification};
use promise::spawn::spawn_into_main_thread;
use std::collections::HashMap;
Expand Down Expand Up @@ -332,10 +332,9 @@ impl SessionHandler {
let tab = mux
.get_tab(tab_id)
.ok_or_else(|| anyhow::anyhow!("tab {tab_id} not found"))?;
tab.set_active_pane(&pane);
tab.set_active_pane(&pane, NotifyMux::No);

mux.record_focus_for_current_identity(pane_id);
mux.notify(mux::MuxNotification::PaneFocused(pane_id));

Ok(Pdu::UnitResponse(UnitResponse {}))
},
Expand Down Expand Up @@ -538,14 +537,14 @@ impl SessionHandler {
if is_zoomed != zoomed {
tab.set_zoomed(false);
if zoomed {
tab.set_active_pane(&pane);
tab.set_active_pane(&pane, NotifyMux::Yes);
tab.set_zoomed(zoomed);
}
}
}
None => {
if zoomed {
tab.set_active_pane(&pane);
tab.set_active_pane(&pane, NotifyMux::Yes);
tab.set_zoomed(zoomed);
}
}
Expand Down

0 comments on commit 2f7ca41

Please sign in to comment.