feat(cwd-pane): Allow to open a new pane from the current working directory #277
Conversation
There was a problem hiding this comment.
Hey @qrasmont - this looks great and I'm sure it was quite a bit of work figuring out all the data paths.
Since this is a bit of a big change I'm going to take a closer look at it in the next few days. Meanwhile, I left one comment. And other than that, I think we don't need a special command for this. Let's make it the default behaviour.
src/common/utils/proc.rs
Outdated
| Ok(cwd) => Some(cwd), | ||
| Err(_) => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move these to the OsApi trait? With one implementation of the trait for linux and one for mac.
This is target depend, on linux we rely on the std lib while on mac os we use the darwin_libproc crate.
Allows to keep of who is the active pane. Update the active pane when spawning a terminal and expose a method to update the active pane.
Allow to open a new pane in the active pane's current directory. - Change the default spawn terminal behavior - os_input_ouput: use a pipe to get the shell pid form child to parrent
Discrease the level of indentation by spliting code in sub functions. Remove unnecessary working_dir match. Clarify pid names.
|
I still need to figure out how I've broken some of the integration tests |
There was a problem hiding this comment.
Hey, great work once again on the changes. Thanks for being patient with my reviews taking a little bit of time and being somewhat fragmented (I'd ideally have liked to give all of this to you in one go, but times are a little hectic in Zellij right now :) ).
I left some comments in the code, and otherwise here's some issues I found so far:
- This doesn't seem to work when I open a new tab. The new terminal there opens up in the default folder.
- If the focused pane is a plugin (eg.
strider), this crashes. To test this, start zellij with the strider layout:zellij --layout strider. Then move focus to the strider file explorer and open a new pane with alt-n. - Similarly to 2, if strider tries to open a new file, zellij crashes. (to reproduce: in strider try to press ENTER on a text file - make sure either the
EDITORorVISUALenv variable on your machine point to a text editor). I imagine this is for the same root cause as 2.
| pub fn spawn_terminal(&mut self, file_to_open: Option<PathBuf>) -> RawFd { | ||
| let (pid_primary, pid_secondary): (RawFd, RawFd) = | ||
| self.os_input.spawn_terminal(file_to_open); | ||
| fn terminal_spawner( |
There was a problem hiding this comment.
Do you think we can either give this a more descriptive name or merge it with spawn_terminal somehow? I find it a little hard to understand the difference between these two methods without being aware of the whole cwd system...
| @@ -1889,6 +1904,9 @@ impl Tab { | |||
| } else { | |||
| self.active_terminal = Some(active_terminal.unwrap().pid()); | |||
| } | |||
| self.send_pty_instructions | |||
| .send(PtyInstruction::UpdateActivePane(self.active_terminal)) | |||
| .unwrap(); | |||
There was a problem hiding this comment.
I think this is the reason some of the tests are failing. Be sure to only send this instruction if the active_terminal has actually changed (eg. here move this inside the else block above).
|
Hey @qrasmont do you still have time to work on this and get it ready to merge? If not, I can pick this up if that would be helpful. |
|
Hey @imsnif I got a little busy this month but I think I can work on this a bit next week. I'll try and take this PR and rebase against main and put it in a new PR to start making the requested changes in. |
|
This is now implemented by #691, |
PR for issue #102
This allows to open a new pane in the current working directory of the active pane (using: Ctrl-p c).
Tested on both Linux and macOS.