-
Notifications
You must be signed in to change notification settings - Fork 351
sdk: basic support for sending and stopping live location shares #3741
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3741 +/- ##
==========================================
- Coverage 84.02% 83.97% -0.06%
==========================================
Files 260 260
Lines 26712 26725 +13
==========================================
- Hits 22446 22441 -5
- Misses 4266 4284 +18 ☔ View full report in Codecov by Sentry. |
4b8b741 to
2f4abd2
Compare
|
Thanks @torrybr please could you sign this off in the PR or commits? Also it looks like there is a conflict to resolve. I'm happy for you to rebase and force push to resolve this. In the meantime I will begin trying to understand this PR to review it. |
Or feel free to leave the merge commit if you prefer. It looks like we will want to squash at the end so either will be fine. |
andybalaam
left a comment
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.
This is looking very nice! Just a few changes I am suggesting. @bnjbvr if you have time to do a quick check over, even better.
| .mount(&server) | ||
| .await; | ||
|
|
||
| mock_sync(&server, &*LIVE_LOCATION_SHARING_SYNC, None).await; |
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.
Is it worth having all this hard-coded JSON? Maybe just adding the beacon_info as we do in the previous test and then stopping it would be adequate.
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.
+1 to this. If we could inline the JSON here too, that'd be great.
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.
After calling room.start_live_location(), how do I resync so that the state store has the beacon_info I need to test? I'm looking for guidance on setting up the mock_sync without using &*LIVE_LOCATION_SHARING_SYNC. This will help me get the other tests in correct shape as well. Any examples would be very helpful! Below is simple example of what im trying to achieve
Updated the start_live_location_share to make use of inline JSON for the sync + check the store for correct saving. Let me know if this is the direction you were thinking and I will update the stop_live_location_test to be more comprehensive. Could I combine these into 1 test (start_and_stop_live_location_share or do you prefer they are separate?
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.
Could I combine these into 1 test (
start_and_stop_live_location_shareor do you prefer they are separate?
I will always recommend separate tests, one for each thing you are testing. It makes it much easier to understand the test, and, critically for me, it makes it easier to see what has gone wrong if a test fails in future.
Feel free to share e.g. test setup code by putting it in a function or even a TestSetup struct if needed.
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.
Updated with comprehensive testing, I can take a second pass to try a TestSetup if desired
91e91e9 to
5ddd6fd
Compare
bnjbvr
left a comment
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.
Exciting!
| .mount(&server) | ||
| .await; | ||
|
|
||
| mock_sync(&server, &*LIVE_LOCATION_SHARING_SYNC, None).await; |
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.
+1 to this. If we could inline the JSON here too, that'd be great.
6ae1932 to
ffa4d48
Compare
I don't see the full "Signed-off-by" line in the PR description. See https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#sign-off and do ask if you have questions. |
ffd8e34 to
9b742fd
Compare
andybalaam
left a comment
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.
A couple more tiny comments, but looking good.
We also need sign-off in the description.
a604eb3 to
35231ea
Compare
aba5797 to
33f58dd
Compare
|
Signed off and ready for final review 👍 |
andybalaam
left a comment
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.
Looks good! Thank you for all your work on this.
This MR introduces basic functionality for sending and stopping live location shares, as outlined in the Matrix Spec Proposal #3489. This foundational support allows users to share their live location, setting the stage for further enhancements.
Key Changes
beacon_infostate event.beacon_infostate event from the state store.These changes were adapted from a previous MR discussion, available for reference here.
Follow-up
Future MRs will expand on this initial implementation, using a layered approach to fully realize the functionality described in the spec above and align with ongoing community discussions.
This does not send any
beaconmessage events. This MR simply introducesbeacon_infosupport with the intention of follow on MRs to build on this basic functionality.Signed-off-by: Torry Brelsford [email protected]