Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interaction between pane swapping / rotating and client domains. #5009

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading