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 CWD, PID and TTY to PaneInfo to make it available to plugins #3800

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

msirringhaus
Copy link
Contributor

Notes:

  • Instead of sending the PaneManifest directly from screen to the plugin, we proxy it through Pty, which then fills out runtime-specific values (tests need to include this additional hop now, too)
  • TTY-detection is currently only available on Linux. It probably would make sense to separate this out into it's own crate (this would also make it easier for contributors to add e.g. macos). Let me know, if that is something I should do.
  • protobuf can't handle PathBufs, so we have to convert them to Strings.

@msirringhaus
Copy link
Contributor Author

I didn't know about #3765 before starting this. But I think this PR includes most of it, too.

@imsnif
Copy link
Member

imsnif commented Dec 19, 2024

Hey @msirringhaus - thanks for putting this together and for your patience!

First thing: while I realize we talked about this and can see the tremendous effort you put into the tty discovery, I would rather not go forward with that part. If this is the easiest way to get it (and we don't have a good way to get it on macos) then I'd rather shelf it for now and move forward with the other bits.

Second: I think the cwd is a great addition but I'm wondering if perhaps we can be a little smarter about it. As opposed to the process id, the cwd is something that can change often and regardless of the Zellij state. Meaning that if I go into a terminal pane and do cd /another/folder, this will not trigger a PaneUpdate event. Those are only triggered from actions Zellij knows about (eg. resizing, opening a new pane, etc.)

Do you think there's a way to somehow listen to these changes - hopefully asynchronously - and trigger an update (not necessarily a PaneUpdate) to the plugins when this happens? This will allow us to do incredibly powerful stuff such as conditionally change the layout depending on the working directory, allow plugins to launch specific processes immediately as you enter a folder, etc. Do you think it's possible?

@msirringhaus
Copy link
Contributor Author

  1. Regarding TTY: I figured as much. I have the code now here: https://github.com/msirringhaus/tty-driver Maybe at some point we'll find someone who can add macOS-support for it.
  2. Hmm... So there is e.g. https://github.com/benfred/remoteprocess which has the ability to give the actual current CWD cross-platform. However, to actually listen to changes may be tricky. On Linux, it's probably relatively easy, as we can watch the /proc/PID/cwd-symlink and react to changes to it. But on MacOS, this seems to be collected with a syscall, so watching would mean polling (unless there are other ways to get this on MacOS. I'm not familiar with that platform at all). As a workaround, we could at least update the CWD on each normal PaneUpdate (e.g. focus switched to another pane, etc.). Or provide functions that do this in the back-end, so plugins can decide themselves if they want to run a background-worker that polls this information.

I'll try to think of something during the upcoming vacation period and hopefully find time to remove the TTY-part of this PR.

@msirringhaus
Copy link
Contributor Author

Rebased this PR on top of new master and removed the TTY-stuff.
Also renamed "cwd" to "startup_cwd" to prepare for "current_cwd" as well, which is not (yet) implemented and I think probably warrants it's own PR. I'm not 100% sure yet, how it will look like, but I think it would be a moderately big PR on it's own.

@imsnif
Copy link
Member

imsnif commented Jan 10, 2025

Hey, thanks for this! This looks good. Two last adjustments please:

  1. I agree with changing the "cwd" field name to be clearer, but how about we call it "last_known_cwd"? I think this is more accurate since this is checked again whenever we populate the PaneManifest
  2. Can we make the pid field an Option too? I think it will be clearer than making it -1 before it's filled. Wdyt?

@imsnif
Copy link
Member

imsnif commented Jan 31, 2025

Hey @msirringhaus - where do we stand with this? I want to release at least the basic features without updating the CWD in the upcoming release. Do you think we can make it happen?

@msirringhaus
Copy link
Contributor Author

Right, sorry about that! Your two change requests should be implemented now

@msirringhaus
Copy link
Contributor Author

Hm, the end to end test failure doesn't look like it has anything to do with my changes?

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