feat(cwd-pane): Keeping the cwd when opening new panes #691
Conversation
Co-authored-by: Quentin Rasmont <[email protected]>
Co-authored-by: Quentin Rasmont <[email protected]>
Co-authored-by: Quentin Rasmont <[email protected]>
Co-authored-by: Quentin Rasmont <[email protected]>
Co-authored-by: Quentin Rasmont <[email protected]>
Co-authored-by: Quentin Rasmont <[email protected]>
There was a problem hiding this comment.
This is looking really good! I tried it out and it completely works as expected. Great work. I also very much appreciate you making an effort to give credit in the commits to the previous PR - that's definitely the right thing to do.
I left some truly minor comments in the PR. Looking forward to having this merged soon.
zellij-server/src/os_input_output.rs
Outdated
| /// # Panics | ||
| /// | ||
| /// This function will panic if a close operation on one of the file descriptors fails or if the | ||
| /// write operation fails. |
There was a problem hiding this comment.
I'm a little iffy about this tbh. This seems like the sort of error we should be able to recover from. If for some reason anything in this process doesn't work, we could open a pane without the CWD like we did before - no?
There was a problem hiding this comment.
Sure, I refactored it to never fail and to just fallback to the previous behavior.
| @@ -145,11 +207,11 @@ fn handle_terminal(cmd: RunCommand, orig_termios: termios::Termios) -> (RawFd, P | |||
| /// This function will panic if both the `EDITOR` and `VISUAL` environment variables are not | |||
| /// set. | |||
| pub fn spawn_terminal( | |||
| terminal_action: Option<TerminalAction>, | |||
| terminal_action: TerminalAction, | |||
There was a problem hiding this comment.
Removing the Option here and using the TerminalAction to move the cwd around is a really elegant solution.
zellij-server/src/os_input_output.rs
Outdated
| @@ -215,7 +275,7 @@ pub trait ServerOsApi: Send + Sync { | |||
| /// Sets the size of the terminal associated to file descriptor `fd`. | |||
| fn set_terminal_size_using_fd(&self, fd: RawFd, cols: u16, rows: u16); | |||
| /// Spawn a new terminal, with a terminal action. | |||
| fn spawn_terminal(&self, terminal_action: Option<TerminalAction>) -> (RawFd, Pid); | |||
| fn spawn_terminal(&self, terminal_action: TerminalAction) -> (RawFd, Pid, RawFd); | |||
There was a problem hiding this comment.
Can we add some comments here explaining what we're returning? It's starting to be a little confusing to me at least :)
There was a problem hiding this comment.
Yeah it was apparently confusing for me too given that the method was actually supposed to be returning a (RawFd, Pid, Pid). I actually refactored it to return a (RawFd, ChildId) and wrote a (hopefully) better documentation explaining what it is. Take a look and see if it's clear to you what's happening.
| Err(_) => None, | ||
| } | ||
| } | ||
| #[cfg(target_os = "linux")] |
There was a problem hiding this comment.
We don't have a lot of (any?) places in the code base that specifically require linux rather than unix. This might be a problem for those using Zellij on eg. FreeBSD. Do you think there can be a unix variant here? If not, can we have a unix variant that doesn't use this feature and will at least compile?
There was a problem hiding this comment.
I resolved this by just returning None for non Linux/MacOS systems. I know very little about alternative Unixes so I don't really feel confident doing anything else here. I did quickly google whether or not FreeBSD has a /proc directory and apparently it does but you have to mount it first. Given that, it's probably not trivial to support everything.
There was a problem hiding this comment.
That's a good solution, but correct me if I'm wrong - right now in those systems it won't compile though right? Because there is no specific function with the correct "is unix" conditional compilation? Or am I missing something?
There was a problem hiding this comment.
Right, I resolved that here 5f13c95 by using:
#[cfg(all(not(target_os = "linux"), not(target_os = "macos")))]Which is admittedly a bit confusing. I couldn't just use #[cfg(target_family = "unix")] because Linux is considered a Unix by Rust, and it'll throw a duplicate definition error. The boolean logic there matches everything that's not either Linux or MacOS, which includes the larger family of Unix OS's Rust supports as well as Windows, Android, ..etc.
There was a problem hiding this comment.
No no, that's good - I missed it before, sorry!
| @@ -54,8 +63,9 @@ impl From<&PtyInstruction> for PtyContext { | |||
| } | |||
|
|
|||
| pub(crate) struct Pty { | |||
| pub active_pane: Option<PaneId>, | |||
There was a problem hiding this comment.
I'm not too wild about this solution. I think essentially duplicating the state between the Pty and the Screen/Tab is a little error prone. One of those things the developer needs to know about.
That being said, given our current architecture of going through the Pty first before going to the Screen when spawning terminals - I can't think of a trivial way around it. If you have an idea, that would be great. Otherwise we can keep this (I want to say "for now", but if I'm being honest I don't see us changing this ever :) ).
There was a problem hiding this comment.
The only thing I can think of (without a large refactor) is adding a screen action with an out parameter via a channel. Is that a different kinda of ugly though? Sorry I'm short on ideas here...
There was a problem hiding this comment.
I think the "right" thing to do would be to change the architecture, or at least formalize some of these paths with a non-blocking shared state somehow... but I feel the right (no-quotation-marks) thing to do for now is to keep your original solution. :)
| @@ -29,6 +33,7 @@ use zellij_utils::{ | |||
|
|
|||
| use async_std::io::ReadExt; | |||
| pub use async_trait::async_trait; | |||
| use byteorder::{BigEndian, ByteOrder}; | |||
There was a problem hiding this comment.
The Unix pipe API we use to send the terminal process id back to our parent process needs a &[u8]. So we have to encode the u32 process id returned by the std::process::Child into a &[u8; 4] array to send it. The byteorder crate does that for us, ByteOrder is just the trait that implements the method to do so.
Instead of panicking when transfering the process id of the forked child command we just return an empty process id.
This will allow Zellij to compile on non Linux/MacOS targets without having an inherited cwd.
The spawn_terminal methods been refactored to use the ChildId struct in order to clarify what the Pid's returned by it are. The documentation for the ChildId struct was improved as well.
|
@spacemaison - I think this is good to go! Only one minor thing I commented about above (the unix conditional compilation) and we can merge this. I'll give this fix priority and merge it ASAP when it's done. |
This is a resurrected pull request for PR #277 and issue #102. It keeps the current working directory when creating new panes/tabs. All tests/lints are passing on my machine.
I might be committing a faux pas here though. I didn't try to rebase #277 on top of the current main because they're was 600+ changes. Instead I added @qrasmont's as co-author to all of my commits. If that's not the appropriate way to handle this I'll be happy to change it.