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

Group Message Kind #217

Merged
merged 15 commits into from
Apr 9, 2024
Merged

Group Message Kind #217

merged 15 commits into from
Apr 9, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Apr 6, 2024

Part of xmtp/libxmtp#522

Adds the message kind to returned message. Then filters for them in the SDK so that GroupMembershipChange Codecs will only get through the messages list if it also has a Membership_Change kind.

@nplasterer nplasterer self-assigned this Apr 6, 2024
@nplasterer nplasterer marked this pull request as ready for review April 8, 2024 19:00
@nplasterer nplasterer requested a review from a team as a code owner April 8, 2024 19:00
@cameronvoell cameronvoell self-requested a review April 8, 2024 23:57
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

Updates + tests all look good. One thing I noticed is that for me this PR branch shows LibXMTP Version empty in the logs for some reason. I'm not sure if maybe bindings were generated in a different way that could cause that. I double tested the version shows up for fine me on main, but not this branch for some reason.

LibXMTP Version: 
Branch: 
Date:

Separate from the version not showing up in logs for some reason - it still feels to me that the libxmtp version should be stored somewhere in the project so that we don't have to build and check logs to see what commit hash of libxmtp android is using. Maybe until we automate that, just adding the libxmtp commit hash to description of PRs that update the bindings would be a good failsafe. wdyt?

@richardhuaaa
Copy link
Contributor

richardhuaaa commented Apr 9, 2024

gen_kotlin.sh creates a libxmtp-version.txt next to the xmtpv3.kt in bindings_ffi/src/uniffi/xmtpv3, if it's helpful maybe we can modify any scripts to copy it over at the same time as xmtpv3.kt and check that file into this repo too?

Also, sorry that the libxmtp version sometimes doesn't show up in code - the build.rs in bindings_ffi doesn't always trigger. It can be fixed by manually running make libxmtp-version and rebuilding the bindings.

@richardhuaaa
Copy link
Contributor

richardhuaaa commented Apr 9, 2024

It might be nice to ensure that make libxmtp-version triggers in cross_build.sh instead of build.rs and gen_kotlin.sh, but the github action would need to be updated too. Hoping I can hand this off to someone? Can explain more if needed

@nplasterer
Copy link
Contributor Author

I copied over the libxmtp txt file. Next time I'm in libxmtp I can take a look at updating the script. 👀

@nplasterer nplasterer merged commit 3c8a080 into main Apr 9, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/message-kind branch April 9, 2024 17:18
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.

4 participants