Skip to content

Conversation

@EmrysMyrddin
Copy link

@EmrysMyrddin EmrysMyrddin commented Mar 23, 2025

Proposal

Description

Adds a the client_id of the client being the source of the pipe message.

Problem

I want to create a plugin that allows to write chars to a specific pane based on it's name.
For this, I want my plugin to react to a pipe message (which can be sent from the CLI or a KeyBind), then tries to find a pane with a matching title in the focused tab.

But, since a pipe message is always sent to all clients, even when it is triggered by a KeyBind, I can't know which tab is focused by the client how sent the KeyBind.

Solution

Add the source client_id to the PipeMessage structure when it's triggered by a KeyBind or a Plugin, or a pane_id when it comes from the CLI.

The pane_id for the CLI pipes is needed because the CLI client doesn't work exactly like a normal client, so it's not possible to keep track of it inside plugins. With the terminal pane_id, I still can know from which tab it comes.

With the source client_id and pane_id, my plugin can know from which tab the pipe message was sent by keeping track of which tab each client is currently focusing.

@imsnif
Copy link
Member

imsnif commented Mar 24, 2025

Hey, thanks for this and good find. I think it makes sense all in all, but I'd rather not add the cli_client_id, since it's misleading (these clients are usually extremely short lived and don't have many of the other guarantees clients have). We could do this by making the field optional in the protobuf declaration.

Also, if you'd like you can add the PaneId (also as an optional field) to the message coming from the CLI to perform the tracking you suggested.

@EmrysMyrddin
Copy link
Author

Yeah, to be honest, I hesitated to do it :-P I noticed that a cli client connection doesn't trigger the SessionUpdate event, so yeah it's not very useful.

You think I should add the PaneId ? I wanted to do i found out that it's not that easy to find out which pane was focused when a KeyBind is triggered, but I can dig through, there must be a way to do it :-)

The addition of the PaneId was in fact why I did the #4095 PR at first :-P Because to add the PaneId to the PipeMessage message, I will probably have to duplicate it once again...

@EmrysMyrddin
Copy link
Author

@imsnif I've updated the PR to only send the client_id when it's not a CLI client.
I've also added the pane_id as you suggested, so that Plugin devs don't have to manually track the client focused pane just to know which pane was focused when a KeyBind was pressed.

Since I'm already modifying the protobuf, perhaps we can merge #4095 into this PR ? Since it's already a breaking change, we can take the opportunity to clean up this.

@imsnif
Copy link
Member

imsnif commented Mar 25, 2025

Hey, from a cursory look this seems fine - thanks! I appreciate the thought with the protobuffs in xtask, but I'd rather not conflate things. Can we remove it for now?

As for the PaneId - if we just add stuff to protobuf files, it's not a breaking change. It's only a breaking change if we change stuff. As I mentioned in the previous PR, moving stuff around might or might not be a breaking change (might also be a change in some implementations and not in others) - but I'd really rather not spend time on this to find out. My resources are limited and I try to spend very deliberate time on changes/additions that will benefit users/plugin developers. Thanks for understanding!

@EmrysMyrddin
Copy link
Author

Ok I see :-) Actually It seems I can use the PaneId defined in event.proto, so it's not mandatory to move things.

For the xtask thing, sure I will remove it. Do you want me to open a new PR for it ?

After testing, it seems the PaneId is not set for KeybindPipe action. Do you know why ? Should I change this too, or you prefere the pane_id to be set only for the CliPipe action ?

@EmrysMyrddin
Copy link
Author

Humm it seems that's really not that easy to know which pane is focused when the KeybindPipe action is triggered.
Perhaps we can just merge this PR as it is, and if someone feel the need to do it, it can be a second PR ?

@EmrysMyrddin
Copy link
Author

@imsnif May I ask you if you have time to take a new look at this PR ?

@imsnif
Copy link
Member

imsnif commented Apr 9, 2025

My apologies, I am juggling a lot of things and am merely Human. Best bet is to try to make this change as small and concise as possible so that I can review and merge it in one go. I'll try to take a look in the next few weeks. Thanks for understanding.

@EmrysMyrddin EmrysMyrddin force-pushed the feat/pipe-client-id branch 4 times, most recently from a265c45 to f4b7497 Compare April 11, 2025 18:49
@EmrysMyrddin
Copy link
Author

No problem, I fully understand that :-) Maintaining alone this kind of project is not an easy task!

I've cleaned up the PR. The changes should be as minimal as I can think about.

@EmrysMyrddin EmrysMyrddin force-pushed the feat/pipe-client-id branch 2 times, most recently from 2dc8335 to f07f1d9 Compare August 4, 2025 18:45
@EmrysMyrddin
Copy link
Author

@imsnif Apologies for the ping here, I've rebased my PR and made sure it includes only the most minimal changes possible.

It's just passing adding the client_id where as parameter so that it can be passed around :-)
Do you think you could have a chance to review this ?

Adds a the `client_id` of the client being the source of the pipe
message.
This allows to take action in the currently focused pane or tab from the
perspective of the client which sent the pipe message.
Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Thank you for your patience! I'll get the CI to run the tests and I also left some comments.

std::thread::sleep(std::time::Duration::from_millis(500));

let _ = plugin_thread_sender.send(PluginInstruction::CliPipe {
pane_id: None,
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe make one of these a Some for the sake of completeness in the tests?

Copy link
Author

Choose a reason for hiding this comment

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

So just one with a fake PaneId::Terminal ?
Or perhaps tow, one PaneId::Terminal and one PaneId::Plugin ?

@EmrysMyrddin
Copy link
Author

Thanks for the review!
I've addressed all the comments and fixed the failing CI 🙂

@EmrysMyrddin
Copy link
Author

By the way I don't know if you want me to clean up the commit history or if you usually squash on merge?

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