-
Notifications
You must be signed in to change notification settings - Fork 58
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
ChatManager to manage sending/receiving of chat messages #143
Conversation
@theomonnom I can't seem to load the shared libs in the test here. Any ideas? |
We need to download the FFI binaries inside the test workflow. Will do it tmrw It is as easy as executing the download-ffi.py script to the resources path |
that worked. Thanks! |
livekit-rtc/livekit/rtc/chat.py
Outdated
_CHAT_UPDATE_TOPIC = "lk-chat-update-topic" | ||
|
||
|
||
class ChatManager: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this class also accumulate chat messages with a sane default way of updating chat messages (i.e. setting deleted from false to true)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I def want to in the future.. just wanted to keep it simple because if we started buffering, we'd need a way to clean it up too to avoid it buffering forever
@davidzhao @theomonnom I did it here as well. Maybe this can be a shared workflow step: https://github.com/livekit/python-sdks/pull/135/files#diff-4fdc48ab490b670c90974725b8e3d399b24c03d6d09563acf8ea87d4113930fbR55 |
No need, it's one internal line. I think you're complicating it inside your build_docs workflow. Only livekit-rtc needs it |
@theomonnom I don't have mypy setup.. It'll be faster if you can please fix lint issues directly. I'd like to get this released so we can update everything else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, lgtm!
nvm last thing is: do we need on_message_updated? |
Thanks! we don't need it right now. Will add this once finalized across clients |
No description provided.