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

Add zoom-pane subcommand #4160

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Conversation

quantonganh
Copy link
Contributor

Hi,

I'm currently working on integrating Helix with WezTerm to perform interactive global search.

The mechanism I'm implementing involves sending a command to the bottom pane when a key is presseed:

case "$1" in
  "fzf")
    split_pane_down
    echo "hx-fzf.sh \$(rg --line-number --column --no-heading --smart-case . | fzf --delimiter : --preview 'bat --style=full --color=always --highlight-line {2} {1}' --preview-window '~3,+{2}+3/2' | awk '{ print \$1 }' | cut -d: -f1,2,3)" | $send_to_bottom_pane
    ;;

Once a selection is made, the file will be opened by Helix in the top pane. Now I want to hide the bottom pane but looks like WezTerm does not support this functionality.

While I'm aware that it's possible to bind a key to toggle the zoom state of the top pane, this requires an additional key press. Furthermore, I'm looking to avoid the option of killing the bottom pane.

Thus, this PR has been initiated to introduce a CLI-based method for toggling the zoom state of the current pane.

@quantonganh quantonganh force-pushed the toggle-pane-zoom-state-cli branch 2 times, most recently from 403a711 to 85ef75d Compare August 21, 2023 03:06
let tab = mux
.get_tab(tab_id)
.ok_or_else(|| anyhow!("no such tab {}", tab_id))?;
tab.toggle_zoom();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for diving in!

There's a nuanced difference between the behavior listed in the CLI and what is actually happening here.

The CLI logic takes a pane_id parameter (defaulting to the pane from the environment), but rather than manipulating the zoom status of that pane, it toggles whichever pane is zoomed in its containing tab, rather than the requested pane.

Note that there is already a SetPaneZoomed PDU in the mux protocol; it also happens to have this same scoping issue.

What I think the forward should be is:

  1. Fix SetPaneZoomed to explicitly check the zoomed status of the pane. To support this, add a method to Tab and TabInner called get_zoomed_pane that returns self.zoomed.clone(). Then the code can use tab.set_zoomed(false) to unset the zoom state directly, and then, if zooming the target pane, tab.set_active_pane() to activate the target tab and tab.set_zoomed(true) to make it zoomed.

  2. With that operation fixed, the rest of this PR doesn't need to modify the codec; all the logic can reside in the CLI.

For the CLI I think it would be better overall if we provided a choice of three actions:

  • Explicitly set the zoom state to either true or false.
  • To toggle the zoom state

eg: name the subcommand zoom-pane and give it parameters: --zoom, --unzoom, and --toggle to select the behavior. I would lean towards --zoom being the default because that feels like the most likely action for zoom-pane to take by default.

The zoom and unzoom cases are straightforward and can call client.set_zoomed and pass the parameters through.

For the toggle case, it's a bit more involved:

  • Use client.list_panes() to obtain tab and pane information (see move_pane_to_new_tab.rs)
  • Resolve the tab_id of the target tab from that information
  • Resolve the pane_id that is active + zoomed
  • If the active + zoomed pane_id matches the target pane, then use client.set_zoomed to set zoomed to false for the target pane
  • Otherwise, use client.set_zoomed to set zoomed to true for the target pane.

Why not add a TogglePaneZoomState PDU and handle this on the server side? Such a toggle action would aggregate multiple functions which would make the mux protocol more complex overall. Having discrete, composable PDUs make things simpler and more flexible in the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just made the changes based on your suggestion. Please review. Thank you.

@quantonganh quantonganh changed the title Toggle the zoom state of the current pane from CLI Add zoom-pane subcommand Aug 28, 2023
Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I have some suggestions to simplify the code a bit!

The freebsd build error is hopefully resolved by cb0e159 and isn't related to your changes.

wezterm-mux-server-impl/src/sessionhandler.rs Outdated Show resolved Hide resolved
wezterm/src/cli/zoom_pane.rs Outdated Show resolved Hide resolved
wezterm/src/cli/zoom_pane.rs Outdated Show resolved Hide resolved
@quantonganh quantonganh requested a review from wez August 29, 2023 04:25
@wez wez merged commit f4db686 into wez:main Aug 29, 2023
16 of 17 checks passed
@wez
Copy link
Owner

wez commented Aug 29, 2023

Thanks!

wez added a commit that referenced this pull request Aug 29, 2023
quantonganh added a commit to quantonganh/helix-wezterm that referenced this pull request Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants