Skip to content

Commit

Permalink
Fix interaction between pane swapping / rotating and client domains.
Browse files Browse the repository at this point in the history
This should address #4200.

Currently pane swapping and rotation only works correctly on local
domains since none of the state is propagated to the remote mux server.

This patch adds a couple of RPCs for rotating panes and for swapping
an active pane with the pane at a given index. Additionally, it renames
the corresponding methods on the `mux::tab` module, prefixing them with
`local_`, adds `remote_` versions of them to Domains and adds a
convenience method on `mux`, mirroring the pattern used for
`move_pane_to_new_tab`, which dispatches to the relevant method.

This incidentally fixes a typo in the Lua API which was previously
always rotating panes in a single direction.
  • Loading branch information
bogdan2412 committed May 20, 2024
1 parent b8f94c4 commit 0f36595
Show file tree
Hide file tree
Showing 15 changed files with 305 additions and 47 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 18 additions & 3 deletions codec/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#![cfg_attr(feature = "cargo-clippy", allow(clippy::range_plus_one))]

use anyhow::{bail, Context as _, Error};
use config::keyassignment::{PaneDirection, ScrollbackEraseMode};
use config::keyassignment::{PaneDirection, RotationDirection, ScrollbackEraseMode};
use mux::client::{ClientId, ClientInfo};
use mux::pane::PaneId;
use mux::renderable::{RenderableDimensions, StableCursorPosition};
Expand Down Expand Up @@ -493,7 +493,7 @@ pdu! {
GetPaneRenderableDimensions: 51,
GetPaneRenderableDimensionsResponse: 52,
PaneFocused: 53,
TabResized: 54,
TabReflowed: 54,
TabAddedToWindow: 55,
TabTitleChanged: 56,
WindowTitleChanged: 57,
Expand All @@ -502,6 +502,8 @@ pdu! {
GetPaneDirection: 60,
GetPaneDirectionResponse: 61,
AdjustPaneSize: 62,
RotatePanes: 63,
SwapActivePaneWithIndex: 64,
}

impl Pdu {
Expand Down Expand Up @@ -803,7 +805,7 @@ pub struct TabAddedToWindow {
}

#[derive(Deserialize, Serialize, PartialEq, Debug)]
pub struct TabResized {
pub struct TabReflowed {
pub tab_id: TabId,
}

Expand Down Expand Up @@ -887,6 +889,19 @@ pub struct ActivatePaneDirection {
pub direction: PaneDirection,
}

#[derive(Deserialize, Serialize, PartialEq, Debug)]
pub struct RotatePanes {
pub pane_id: PaneId,
pub direction: RotationDirection,
}

#[derive(Deserialize, Serialize, PartialEq, Debug)]
pub struct SwapActivePaneWithIndex {
pub active_pane_id: PaneId,
pub with_pane_index: usize,
pub keep_focus: bool,
}

#[derive(Deserialize, Serialize, PartialEq, Debug)]
pub struct GetPaneRenderChanges {
pub pane_id: PaneId,
Expand Down
2 changes: 1 addition & 1 deletion config/src/keyassignment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ impl Default for SplitSize {
}
}

#[derive(Debug, Clone, PartialEq, Eq, FromDynamic, ToDynamic)]
#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, FromDynamic, ToDynamic)]
pub enum RotationDirection {
Clockwise,
CounterClockwise,
Expand Down
1 change: 1 addition & 0 deletions lua-api-crates/mux/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ log = "0.4"
luahelper = { path = "../../luahelper" }
parking_lot = "0.12"
portable-pty = { path = "../../pty" }
promise = { path = "../../promise" }
smol = "2.0"
termwiz = { path = "../../termwiz" }
termwiz-funcs = { path = "../termwiz-funcs" }
Expand Down
26 changes: 23 additions & 3 deletions lua-api-crates/mux/src/tab.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use config::keyassignment::PaneDirection;
use config::keyassignment::{PaneDirection, RotationDirection};

use super::*;
use luahelper::mlua::Value;
Expand Down Expand Up @@ -113,14 +113,34 @@ impl UserData for MuxTab {
methods.add_method("rotate_counter_clockwise", |_, this, _: ()| {
let mux = get_mux()?;
let tab = this.resolve(&mux)?;
tab.rotate_counter_clockwise();

let tab_id = tab.tab_id();
let direction = RotationDirection::CounterClockwise;
promise::spawn::spawn(async move {
let mux = Mux::get();
if let Err(err) = mux.rotate_panes(tab_id, direction).await {
log::error!("Unable to rotate panes: {:#}", err);
}
})
.detach();

Ok(())
});

methods.add_method("rotate_clockwise", |_, this, _: ()| {
let mux = get_mux()?;
let tab = this.resolve(&mux)?;
tab.rotate_counter_clockwise();

let tab_id = tab.tab_id();
let direction = RotationDirection::CounterClockwise;
promise::spawn::spawn(async move {
let mux = Mux::get();
if let Err(err) = mux.rotate_panes(tab_id, direction).await {
log::error!("Unable to rotate panes: {:#}", err);
}
})
.detach();

Ok(())
});

Expand Down
29 changes: 27 additions & 2 deletions mux/src/domain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::window::WindowId;
use crate::Mux;
use anyhow::{bail, Context, Error};
use async_trait::async_trait;
use config::keyassignment::{SpawnCommand, SpawnTabDomain};
use config::keyassignment::{RotationDirection, SpawnCommand, SpawnTabDomain};
use config::{configuration, ExecDomain, SerialDomain, ValueOrFunc, WslDomain};
use downcast_rs::{impl_downcast, Downcast};
use parking_lot::Mutex;
Expand Down Expand Up @@ -142,7 +142,7 @@ pub trait Domain: Downcast + Send + Sync {
/// is being moved to give the domain a chance to handle the movement.
/// If this method returns Ok(None), then the mux will handle the
/// movement itself by mutating its local Tabs and Windows.
async fn move_pane_to_new_tab(
async fn remote_move_pane_to_new_tab(
&self,
_pane_id: PaneId,
_window_id: Option<WindowId>,
Expand All @@ -151,6 +151,31 @@ pub trait Domain: Downcast + Send + Sync {
Ok(None)
}

/// The mux will call this method on the domain of the panes that are being
/// rotated to give the domain a chance to handle the movement. If this
/// method returns Ok(false), then the mux will handle the movement itself
/// by mutating its local Tabs and Windows.
async fn remote_rotate_panes(
&self,
_pane_id: PaneId,
_direction: RotationDirection,
) -> anyhow::Result<bool> {
Ok(false)
}

/// The mux will call this method on the domain of the pane that is being
/// swapped to give the domain a chance to handle the movement. If this
/// method returns Ok(false), then the mux will handle the movement itself
/// by mutating its local Tabs and Windows.
async fn remote_swap_active_pane_with_index(
&self,
_active_pane_id: PaneId,
_with_pane_index: usize,
_keep_focus: bool,
) -> anyhow::Result<bool> {
Ok(false)
}

/// Returns false if the `spawn` method will never succeed.
/// There are some internal placeholder domains that are
/// pre-created with local UI that we do not want to allow
Expand Down
71 changes: 68 additions & 3 deletions mux/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::ssh_agent::AgentProxy;
use crate::tab::{SplitRequest, Tab, TabId};
use crate::window::{Window, WindowId};
use anyhow::{anyhow, Context, Error};
use config::keyassignment::SpawnTabDomain;
use config::keyassignment::{RotationDirection, SpawnTabDomain};
use config::{configuration, ExitBehavior, GuiPosition};
use domain::{Domain, DomainId, DomainState, SplitSource};
use filedescriptor::{poll, pollfd, socketpair, AsRawSocketDescriptor, FileDescriptor, POLLIN};
Expand Down Expand Up @@ -80,7 +80,7 @@ pub enum MuxNotification {
window_id: WindowId,
},
PaneFocused(PaneId),
TabResized(TabId),
TabReflowed(TabId),
TabTitleChanged {
tab_id: TabId,
title: String,
Expand Down Expand Up @@ -1245,7 +1245,7 @@ impl Mux {
.ok_or_else(|| anyhow::anyhow!("domain {domain_id} of pane {pane_id} not found"))?;

if let Some((tab, window_id)) = domain
.move_pane_to_new_tab(pane_id, window_id, workspace_for_new_window.clone())
.remote_move_pane_to_new_tab(pane_id, window_id, workspace_for_new_window.clone())
.await?
{
return Ok((tab, window_id));
Expand Down Expand Up @@ -1289,6 +1289,71 @@ impl Mux {
Ok((tab, window_id))
}

pub async fn rotate_panes(
&self,
tab_id: TabId,
direction: RotationDirection,
) -> anyhow::Result<()> {
let tab = match self.get_tab(tab_id) {
Some(tab) => tab,
None => anyhow::bail!("Invalid tab id {}", tab_id),
};

// This makes the assumption that a tab contains only panes from a single local domain,
// though that is also an assumption that ClientDomain makes when syncing tab panes.
let tab_panes = tab.iter_panes();
let pos_pane = match tab_panes.iter().nth(0) {
Some(pos_pane) => pos_pane,
None => anyhow::bail!("Tab contains no panes: {}", tab_id),
};

let pane_id = pos_pane.pane.pane_id();
let domain_id = pos_pane.pane.domain_id();

let domain = self
.get_domain(domain_id)
.ok_or_else(|| anyhow::anyhow!("domain {domain_id} of tab {tab_id} not found"))?;

if domain.remote_rotate_panes(pane_id, direction).await? {
return Ok(());
}

match direction {
RotationDirection::Clockwise => tab.local_rotate_clockwise(),
RotationDirection::CounterClockwise => tab.local_rotate_counter_clockwise(),
}
Ok(())
}

pub async fn swap_active_pane_with_index(
&self,
active_pane_id: PaneId,
with_pane_index: usize,
keep_focus: bool,
) -> anyhow::Result<()> {
let (domain_id, _window_id, tab_id) = self
.resolve_pane_id(active_pane_id)
.ok_or_else(|| anyhow::anyhow!("pane {} not found", active_pane_id))?;

let domain = self.get_domain(domain_id).ok_or_else(|| {
anyhow::anyhow!("domain {domain_id} of pane {active_pane_id} not found")
})?;

if domain
.remote_swap_active_pane_with_index(active_pane_id, with_pane_index, keep_focus)
.await?
{
return Ok(());
}

let tab = match self.get_tab(tab_id) {
Some(tab) => tab,
None => anyhow::bail!("Invalid tab id {}", tab_id),
};

tab.local_swap_active_with_index(with_pane_index, keep_focus);
Ok(())
}
pub async fn spawn_tab_or_window(
&self,
window_id: Option<WindowId>,
Expand Down
31 changes: 16 additions & 15 deletions mux/src/tab.rs
Original file line number Diff line number Diff line change
Expand Up @@ -581,12 +581,12 @@ impl Tab {
self.inner.lock().iter_panes_ignoring_zoom()
}

pub fn rotate_counter_clockwise(&self) {
self.inner.lock().rotate_counter_clockwise()
pub fn local_rotate_counter_clockwise(&self) {
self.inner.lock().local_rotate_counter_clockwise()
}

pub fn rotate_clockwise(&self) {
self.inner.lock().rotate_clockwise()
pub fn local_rotate_clockwise(&self) {
self.inner.lock().local_rotate_clockwise()
}

pub fn iter_splits(&self) -> Vec<PositionedSplit> {
Expand Down Expand Up @@ -714,10 +714,10 @@ impl Tab {
}

/// Swap the active pane with the specified pane_index
pub fn swap_active_with_index(&self, pane_index: usize, keep_focus: bool) -> Option<()> {
pub fn local_swap_active_with_index(&self, pane_index: usize, keep_focus: bool) -> Option<()> {
self.inner
.lock()
.swap_active_with_index(pane_index, keep_focus)
.local_swap_active_with_index(pane_index, keep_focus)
}

/// Computes the size of the pane that would result if the specified
Expand Down Expand Up @@ -908,7 +908,7 @@ impl TabInner {
self.zoomed.replace(pane);
}
}
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn contains_pane(&self, pane: PaneId) -> bool {
Expand Down Expand Up @@ -937,7 +937,7 @@ impl TabInner {
self.iter_panes_impl(false)
}

fn rotate_counter_clockwise(&mut self) {
fn local_rotate_counter_clockwise(&mut self) {
let panes = self.iter_panes_ignoring_zoom();
if panes.is_empty() {
// Shouldn't happen, but we check for this here so that the
Expand Down Expand Up @@ -966,9 +966,10 @@ impl TabInner {
}
}
}
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn rotate_clockwise(&mut self) {
fn local_rotate_clockwise(&mut self) {
let panes = self.iter_panes_ignoring_zoom();
if panes.is_empty() {
// Shouldn't happen, but we check for this here so that the
Expand Down Expand Up @@ -997,7 +998,7 @@ impl TabInner {
}
}
}
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn iter_panes_impl(&mut self, respect_zoom_state: bool) -> Vec<PositionedPane> {
Expand Down Expand Up @@ -1179,7 +1180,7 @@ impl TabInner {
apply_sizes_from_splits(self.pane.as_mut().unwrap(), &size);
}

Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn apply_pane_size(&mut self, pane_size: TerminalSize, cursor: &mut Cursor) {
Expand Down Expand Up @@ -1255,7 +1256,7 @@ impl TabInner {
self.size = size;
}
}
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn resize_split_by(&mut self, split_index: usize, delta: isize) {
Expand Down Expand Up @@ -1288,7 +1289,7 @@ impl TabInner {
// Now cursor is looking at the split
self.adjust_node_at_cursor(&mut cursor, delta);
self.cascade_size_from_cursor(cursor);
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn adjust_node_at_cursor(&mut self, cursor: &mut Cursor, delta: isize) {
Expand Down Expand Up @@ -1371,7 +1372,7 @@ impl TabInner {
}
}
}
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabResized(self.id)));
Mux::try_get().map(|mux| mux.notify(MuxNotification::TabReflowed(self.id)));
}

fn adjust_pane_size(&mut self, direction: PaneDirection, amount: usize) {
Expand Down Expand Up @@ -1807,7 +1808,7 @@ impl TabInner {
cell_dimensions(&self.size)
}

fn swap_active_with_index(&mut self, pane_index: usize, keep_focus: bool) -> Option<()> {
fn local_swap_active_with_index(&mut self, pane_index: usize, keep_focus: bool) -> Option<()> {
let active_idx = self.get_active_idx();
let mut pane = self.get_active_pane()?;
log::trace!(
Expand Down
Loading

0 comments on commit 0f36595

Please sign in to comment.