Skip to content

Comments

Add capture pane functionality#59

Open
max-legrand wants to merge 6 commits intorockorager:mainfrom
max-legrand:capture-pane
Open

Add capture pane functionality#59
max-legrand wants to merge 6 commits intorockorager:mainfrom
max-legrand:capture-pane

Conversation

@max-legrand
Copy link
Contributor

Copy link
Owner

@rockorager rockorager left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Capture pane is a useful feature (issue #57).

However, I think the implementation conflates two separate concerns:

  1. Capturing the pane content - the core feature
  2. Opening in an editor - one specific use case

Issues with current approach

1. Timeout is fragile

prise.set_timeout(500, function()

This waits 500ms hoping the server responded and client wrote the file. Large scrollbacks could take longer, or it could be unnecessarily slow for small ones.

2. Should be event-driven

Instead of a timeout, the client should emit an event when the capture is complete:

{ type = "capture_pane_complete", path = "/tmp/...", content = "..." }

Then Lua handles it reactively.

3. Editor logic shouldn't be baked in

The PR hardcodes opening $EDITOR in a new tab with auto-close. This is one specific workflow, but users might want to:

  • Pipe to fzf/grep
  • Send to clipboard
  • Open in a floating window
  • Append to a log file
  • etc.

Suggested approach

Make capture_pane a simple primitive:

  • pty:capture_pane() triggers capture
  • Client emits event when complete: { type = "capture_pane_complete", pty_id = ..., content = "..." }
  • User handles the event in their config however they want

The "open in editor" workflow can be a documented example in the README showing how to wire it up, not built into the core.

This keeps the API composable and lets users build their own workflows on top of the primitive.

@max-legrand
Copy link
Contributor Author

Unsure exactly when I'll be able to address the feedback on this (or #60). If these are sitting for a while and it is quicker for someone else to make the necessary changes to get this merged I'm more than happy to close this PR out in favor of having the changes be included sooner, but I will try to find the time to make changes based on the review comments.

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