Skip to content

Remove ClientChannelView #286

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

Merged
merged 5 commits into from
Mar 14, 2025
Merged

Remove ClientChannelView #286

merged 5 commits into from
Mar 14, 2025

Conversation

jtbandes
Copy link
Member

@jtbandes jtbandes commented Mar 14, 2025

Changelog

Rust: callbacks for on_client_advertise, on_client_unadvertise, and on_message_data now receive a &ClientChannel instead of a ClientChannelView.
Python: callback for on_client_advertise now receives a ClientChannel instead of a ChannelView; on_client_unadvertise and on_message_data now receive a client_channel_id instead of a ChannelView.

Docs

None

Description

Motivation: all the information about the client channel should be accessible to users in the callbacks.

Thoughts?

@jtbandes jtbandes requested review from eloff, bryfox and gasmith March 14, 2025 00:25
Copy link
Contributor

@eloff eloff left a comment

Choose a reason for hiding this comment

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

I like the change, it's how I originally wrote it and then changed my mind in the code review. I think the worry was exposing websocket protocol specific types in the callbacks, but those callbacks are very websocket and sometimes ros specific anyway.

For Python I see you opted not to expose the additional info other than the topic and id. We'll have to remember to come back and fix that. Unfortunately it all means allocating and copying in Python, there's no way around that - because Python is able to store a reference to ClientChannel in a way that escapes the callback. Rust cannot.

I think C and C++ have the same problems as Python, not sure how we deal with that there in performance may matter land. Maybe we give them a pointer or reference to an object with borrowed data (string_view or similar for c++?) and document that they have to copy it if they want to keep it after the callback returns.

@jtbandes
Copy link
Member Author

Updated to expose the info to Python as well.

@jtbandes jtbandes requested a review from eloff March 14, 2025 21:04
Copy link
Contributor

@bryfox bryfox left a comment

Choose a reason for hiding this comment

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

I only took a quick look at the rust source since it'd been reviewed already, but this looks good to me.

@jtbandes jtbandes merged commit b6ab4df into main Mar 14, 2025
38 checks passed
@jtbandes jtbandes deleted the jacob/client-channel-callback branch March 14, 2025 22:05
jtbandes added a commit that referenced this pull request Mar 19, 2025
### Changelog
C/C++: added callbacks for client advertise, publish, and unadvertise

### Docs

None

### Description

~Depends on #286~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants