Float Pane #5576

e82eric · 2024-06-17T08:14:21Z

Hi, I wanted to make an attempt at #270

This probably needs quite a bit of iteration (this is my first attempt at rust), but wanted to create the pull request to see if this is something you would be open to adding and to see if I am on the right track.

derekthecool · 2024-06-29T02:32:31Z

Nice work! Demo video looks good.

mathjiajia · 2024-07-01T03:10:50Z

it would be nice to have a border for this floating pane

e82eric · 2024-07-03T04:43:56Z

Updated with border.

Configs:

config.float_pane_padding = {
  left = '10%',
  right = '10%',
  top = '10%',
  bottom = '10%',
}

config.float_pane_border = {
  left_width = '0.5cell',
  right_width = '0.5cell',
  bottom_height = '0.25cell',
  top_height = '0.25cell',
  left_color = '#665c54',
  right_color = '#665c54',
  bottom_color = '#665c54',
  top_color = '#665c54',
}

e82eric · 2024-07-06T00:15:06Z

I think this is ready to be reviewed now.

Pajn · 2024-07-13T08:32:12Z

Thanks a lot, this is the one feature I've been missing since transition from tmux to wezterm mux 🙏🏼

Tested this and it works really well, just commenting with some small issues I've seen:

The float panes all open in my home directory instead of the current directory like split panes and new tabs do
The pane selection UI is hard to read if having no split panes and one floating since the underlying pane and the float panes letter overlaps
If moving all non floating panes to a new tab or window the floating pane is shown on an otherwise invisible window, would be nice to either disallow this or de-float the floating pane

A very nice action to have but definitely not required for this to be super useful on its own, is to be able to toggle the floating status of a pane so you can transition a floating pane to a split pane and vice verse.

e82eric · 2024-07-14T07:06:46Z

@Pajn I think I have the working directory issue fixed now.

For the pane selection my intention was to treat the float like a overlay and turn any selection operations into no-ops and prevent the mouse from selecting a pane (I think there were some scenarios that I missed like the selection UI). I pushed an update that I think turn those into nops now. This may not be the right strategy, is there a scenario where you would want to be able to select/interact with the panes underneath the float?

I think I have it updated so that closing the pane underneath the float is blocked now. Which I think will prevent the scenario where only the float is showing.

I tried out being able to move the float to a split today. It seemed to work, I pushed it to this branch if you want to give it a try. I didn't want to include it in this pull request to keep the scope down.
https://github.com/e82eric/wezterm/tree/move-float-to-split

This is what it looked like.

Pajn · 2024-07-14T07:36:54Z

Wow! I was just starting to familiarize myself with the code to see if I could help but not at that speed :)
Very quick testing have all the issues I experienced fixed.

This may not be the right strategy, is there a scenario where you would want to be able to select/interact with the panes underneath the float?

I was mostly just trying to break it to see where the limitations where. The one possible usecase I can see is to move one background pane to another tab/window if you need access to it after realizing the float pane was longer lived than you intended. However I think that's better solved by being able to transition the float pane to a split anyway. And disabling those actions when in a float definitely avoids a whole class of potential problems.

I didn't want to include it in this pull request to keep the scope down.

Totes! I was just excited. Thanks a lot for the branch though, will definitely start playing with it.
And if there is anything I can do to help get this merged, let me know.

Suri312006 · 2024-07-14T17:38:46Z

i really like the work man, good stuff been wanting this feature, as im coming from zellij that has a really nice implementation of floating panes

wezterm-gui/src/commands.rs

e82eric · 2024-08-09T01:27:28Z

Thanks! updated

dev-lop36 · 2024-09-17T15:12:08Z

What is the status of this PR? Really great feature&work btw!

e82eric · 2024-09-18T04:49:40Z

Thanks! I think this is more or less still pending review. I am happy to put in the work to get this across the the finish line, but don't want bug Wez about it, I am sure he has a million priorities. In the mean time if anyone wants to give it a try and notice any issues or want to review the code, I can work through those.

wez

Many thanks for diving into this, and doing so deeply!

I haven't had the bandwidth to look at this in a great deal of detail, just skimmed here on GH without trying to run it, but have a few comments/suggestions.

There are a number of places with TODOs and comments about refactoring to reduce duplication: I would like to avoid having duplicate or almost identical code for the floating pane stuff, so I would appreciate seeing that tackled before we get to merging this.

I think there are perhaps some other bigger picture or weird edge-case questions around this that I think it would be good to have answers for; there may be more of these in the long run, but these are the ones more or less off of the top of my head:

How does and how should the floating pane be reflected in wezterm cli list?
If you create a floating pane against a multiplexer and detach, does re-attaching preserve the float?
If you have one wezterm connected to a mux, then attach a second wezterm instance to that same mux and spawn a float in one, does the float instantly appear in the other one?
How does the scrollbar UI/UX work with the floating pane? I'm OK if "it doesn't" is the answer if we document that it doesn't, and ensure that it is well-behaved.
Are there any special gotchas when it comes to using the floating pane?
Is it necessary to add an explicit kill-floating-pane action, or are the existing actions sufficient?
How does the floating pane get exposed to the various window/tab styling events in lua, and is it likely to cause problems with existing setups?

wez · 2024-09-21T19:09:30Z

mux/src/tab.rs

@@ -1861,6 +1945,42 @@ impl TabInner {
        None
    }

+    fn get_float_size(&self) -> TerminalSize {


To me, get_float_size implies getting the size of the current float, but this is computing the desired size, so I think it should be renamed to make it more clear

Suggested change

fn get_float_size(&self) -> TerminalSize {

fn compute_float_size(&self) -> TerminalSize {

wez · 2024-09-21T19:10:52Z

mux/src/tab.rs

+        {
+            pane.resize(float_size)?;
+        }
+
+        Ok(())


Suggested change

{

pane.resize(float_size)?;

}

Ok(())

pane.resize(float_size)?;

Ok(())

wez · 2024-09-21T19:11:45Z

wezterm-gui/src/commands.rs

+                          domain: SpawnTabDomain::CurrentPaneDomain,
+                          ..
+                      }) => CommandDef {
+            brief: label_string(action, "Create a float pane".to_string()).into(),


Suggested change

brief: label_string(action, "Create a float pane".to_string()).into(),

brief: label_string(action, "Create a floating pane".to_string()).into(),

wez · 2024-09-21T19:11:59Z

wezterm-gui/src/commands.rs

+                          ..
+                      }) => CommandDef {
+            brief: label_string(action, "Create a float pane".to_string()).into(),
+            doc: "Create a float pane"


Suggested change

doc: "Create a float pane"

doc: "Create a floating pane"

wez · 2024-09-21T19:12:13Z

wezterm-gui/src/commands.rs

@@ -1501,6 +1518,15 @@ pub fn derive_command_from_key_assignment(action: &KeyAssignment) -> Option<Comm
            menubar: &[],
            icon: Some("cod_split_vertical"),
        },
+        FloatPane(_) => CommandDef {
+            brief: label_string(action, "Create a float pane".to_string()).into(),


Suggested change

brief: label_string(action, "Create a float pane".to_string()).into(),

brief: label_string(action, "Create a floating pane".to_string()).into(),

wez · 2024-09-21T19:12:21Z

wezterm-gui/src/commands.rs

@@ -1501,6 +1518,15 @@ pub fn derive_command_from_key_assignment(action: &KeyAssignment) -> Option<Comm
            menubar: &[],
            icon: Some("cod_split_vertical"),
        },
+        FloatPane(_) => CommandDef {
+            brief: label_string(action, "Create a float pane".to_string()).into(),
+            doc: "Create a float pane"


Suggested change

doc: "Create a float pane"

doc: "Create a floating pane"

wez · 2024-09-21T19:13:36Z

wezterm-gui/src/termwindow/mod.rs

-        if let Some(pane) = self.get_active_pane_or_overlay() {
-            pane.focus_changed(focused);
-        }
-


removing this logic is probably a regression?

reverted. (sorry, I should have caught this).

wez · 2024-09-21T19:20:04Z

wezterm-gui/src/termwindow/mouseevent.rs

+                && position.row as usize >= float_pane.top && position.row as usize <= float_pane.top + float_pane.height {
+                vec![float_pane]
+            } else {
+                return


does this return prevent mouse handling from working in the other panes when a float is present?
Is that intentional? If so, please add a comment around this block to explain this, as it isn't clear from the code.

Also, I prefer an early return style to avoid lots of nested blocks, something like this:

let mouse_in_float = position.column >= float_pane.left && position.column <= float_pane.left + float_pane.width && position.row as usize >= float_pane.top && position.row as usize <= float_pane.top + float_pane.height; if !mouse_in_float { // Mouse events are not dispatched to the other panes when // a floating pane is active, because REASON return; } vec![float_pane]

wez · 2024-09-21T19:22:18Z

wezterm/src/cli/mod.rs

+        name = "float-pane",
+        rename_all = "kebab",
+        trailing_var_arg = true,
+        about = "spawn a float pane"


Suggested change

about = "spawn a float pane"

about = "spawn a floating pane"

wez · 2024-09-21T19:22:53Z

wezterm/src/cli/split_pane.rs

+
+#[derive(Debug, Parser, Clone)]
+pub struct FloatPane {
+    /// Specify the pane that should be split.


this comment doesn't seem to fit the action

e82eric · 2024-09-22T00:41:16Z

Thanks Wez! I pushed a commit to address the initial feedback.

I think the multiplexing, scrollbar and lua events are probably all gaps. Will start working through those and will @ you when I get through them. Marking the PR as draft until I get through them.

Open questions - Is (PaneNode, PaneNode) ok? Or should it be a type? - I think the tuple matches what Zellij has. - Does the output need anything to indicate that the pane is a float? - I don't think Zellij supports list panes (It looked like there is a PR) - It looked like tmux popups are not persistent so I don't think it is applicable here

a single mux

rotate_counter_clockwise

e82eric force-pushed the float-pane branch 3 times, most recently from 9bd3c95 to 03eabc7 Compare June 23, 2024 16:38

e82eric added 8 commits June 23, 2024 14:30

Attempt at float pane

5ee02e4

Try to prevent changes when a float is being shown

518e0c8

Trying to get cli float-pane to work

be7d669

Reverted changes to allow float pane size configuration

6da5f03

Experiment with float borders

83b7974

float padding config

44c8a21

Remove float pane from tab.panes

3bcf229

Removed pane_id where not needed

f9de55f

e82eric added 3 commits July 2, 2024 00:32

More accurate float borders

543af2b

Handle Resize

275567a

Better mouse handling

203e362

e82eric force-pushed the float-pane branch from 03eabc7 to 0da8a62 Compare July 3, 2024 04:26

Clean up

a3e7444

e82eric force-pushed the float-pane branch from 0da8a62 to a3e7444 Compare July 3, 2024 04:30

e82eric added 2 commits July 13, 2024 07:44

Update float-pane to use cwd instead of home directory

e8ee2a8

prevent running pane manipulation actions when float pane is active

128a1ca

MLFlexer reviewed Aug 8, 2024

View reviewed changes

wezterm-gui/src/commands.rs Outdated Show resolved Hide resolved

MLFlexer reviewed Aug 8, 2024

View reviewed changes

wezterm-gui/src/commands.rs Outdated Show resolved Hide resolved

Updated float pane icons

adba9f2

mjs mentioned this pull request Sep 5, 2024

add support for popup window #270

Open

eotsn mentioned this pull request Sep 10, 2024

Return to previous tab in visiting order when the current tab is closed #2997

Open

wez reviewed Sep 21, 2024

View reviewed changes

Floating pane: address initial feedback

cd0ee5f

e82eric marked this pull request as draft September 22, 2024 00:42

e82eric added 6 commits September 22, 2024 10:18

Fix for not creating float pane in multiplexer

e567786

Re-attach the floating pane

7b20a18

Fix for floating pane not being removed from all connected wezterms for

29d3988

a single mux

Refactored remove_float_pane

5ad7fe6

Fix for floating pane not working with rotate_clockwise and

b2e2b79

rotate_counter_clockwise

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Float Pane #5576

Float Pane #5576

e82eric commented Jun 17, 2024

derekthecool commented Jun 29, 2024

mathjiajia commented Jul 1, 2024

e82eric commented Jul 3, 2024 •

edited

Loading

e82eric commented Jul 6, 2024

Pajn commented Jul 13, 2024

e82eric commented Jul 14, 2024

Pajn commented Jul 14, 2024 •

edited

Loading

Suri312006 commented Jul 14, 2024

e82eric commented Aug 9, 2024

dev-lop36 commented Sep 17, 2024

e82eric commented Sep 18, 2024

wez left a comment

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

e82eric Sep 22, 2024

wez Sep 21, 2024

wez Sep 21, 2024

wez Sep 21, 2024

e82eric commented Sep 22, 2024

	fn get_float_size(&self) -> TerminalSize {
	fn compute_float_size(&self) -> TerminalSize {

	brief: label_string(action, "Create a float pane".to_string()).into(),
	brief: label_string(action, "Create a floating pane".to_string()).into(),

Float Pane #5576

Are you sure you want to change the base?

Float Pane #5576

Conversation

e82eric commented Jun 17, 2024

derekthecool commented Jun 29, 2024

mathjiajia commented Jul 1, 2024

e82eric commented Jul 3, 2024 • edited Loading

e82eric commented Jul 6, 2024

Pajn commented Jul 13, 2024

e82eric commented Jul 14, 2024

Pajn commented Jul 14, 2024 • edited Loading

Suri312006 commented Jul 14, 2024

e82eric commented Aug 9, 2024

dev-lop36 commented Sep 17, 2024

e82eric commented Sep 18, 2024

wez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e82eric commented Sep 22, 2024

e82eric commented Jul 3, 2024 •

edited

Loading

Pajn commented Jul 14, 2024 •

edited

Loading