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

expose sync user subscribe in the c-api #7302

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

nicola-cab
Copy link
Member

@nicola-cab nicola-cab commented Jan 30, 2024

What, How & Why?

Fixes: #7301

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@cla-bot cla-bot bot added the cla: yes label Jan 30, 2024
@nicola-cab nicola-cab force-pushed the nc/expose_sync_user_subscribe branch 3 times, most recently from c24d7d4 to d743f57 Compare January 30, 2024 12:14
Copy link

coveralls-official bot commented Jan 30, 2024

Pull Request Test Coverage Report for Build nicola.cabiddu_1344

  • 0 of 22 (100.0%) changed or added relevant lines in 4 files are covered.
  • 81 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.01%) to 91.855%

Files with Coverage Reduction New Missed Lines %
src/realm/array_key.cpp 1 97.53%
src/realm/sync/network/network.cpp 1 89.66%
test/test_index_string.cpp 1 94.13%
src/realm/table_view.cpp 2 94.18%
test/test_lang_bind_helper.cpp 2 93.31%
src/realm/sync/noinst/client_impl_base.cpp 3 85.85%
test/test_thread.cpp 3 66.67%
src/realm/sync/transform.cpp 4 63.08%
src/realm/sync/network/network.hpp 7 85.41%
src/realm/sync/noinst/server/server_history.cpp 7 67.66%
Totals Coverage Status
Change from base Build 2001: -0.01%
Covered Lines: 235334
Relevant Lines: 256203

💛 - Coveralls

@@ -615,6 +615,9 @@ TEST_CASE("C API (non-database)", "[c_api]") {
},
&sync_user, user_data_free);

auto user_state = [](realm_userdata_t, realm_user_state_e) {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should check the user logs in (and other state changes)

src/realm.h Outdated

// register callback for tracking user status.
RLM_API realm_user_subscription_token_t*
realm_user_state_change_register_callback(realm_user_t*, realm_user_changed_callback_t, realm_userdata_t userdata,
Copy link
Collaborator

Choose a reason for hiding this comment

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

realm_sync_user_register_state_changed_callback?

src/realm.h Outdated
typedef struct realm_async_open_task_progress_notification_token realm_async_open_task_progress_notification_token_t;
typedef struct realm_sync_session_connection_state_notification_token
realm_sync_session_connection_state_notification_token_t;
typedef struct realm_user_subscription_token realm_user_subscription_token_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sync_user

src/realm.h Outdated
@@ -3495,9 +3495,13 @@ typedef enum realm_flx_sync_subscription_set_state {
typedef void (*realm_sync_on_subscription_state_changed_t)(realm_userdata_t userdata,
realm_flx_sync_subscription_set_state_e state);

typedef void (*realm_user_changed_callback_t)(realm_userdata_t userdata, realm_user_state_e s);
Copy link
Collaborator

Choose a reason for hiding this comment

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

realm_sync_user_state_changed_t?

CHANGELOG.md Outdated
@@ -2,6 +2,7 @@

### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* Add support in the C API for receiving a notification when sync user status changes. ([#7302](https://github.com/realm/realm-core/pull/7302))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: status -> state

src/realm.h Outdated
@@ -3894,6 +3898,12 @@ RLM_API realm_sync_session_connection_state_notification_token_t* realm_sync_ses
realm_sync_session_t*, realm_sync_progress_func_t, realm_sync_progress_direction_e, bool is_streaming,
realm_userdata_t userdata, realm_free_userdata_func_t userdata_free) RLM_API_NOEXCEPT;


// register callback for tracking user status.
Copy link
Collaborator

Choose a reason for hiding this comment

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

// @return a notification token object. Dispose it to stop receiving notifications.

src/realm.h Outdated
@@ -3495,9 +3495,13 @@ typedef enum realm_flx_sync_subscription_set_state {
typedef void (*realm_sync_on_subscription_state_changed_t)(realm_userdata_t userdata,
realm_flx_sync_subscription_set_state_e state);

typedef void (*realm_user_changed_callback_t)(realm_userdata_t userdata, realm_user_state_e s);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if realm_user_t* should be an argument to be able to easily distinguish between different user notifications. I guess realm_userdata_t could be used for that?

Copy link
Member Author

@nicola-cab nicola-cab Feb 1, 2024

Choose a reason for hiding this comment

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

yes, the sdks can pass whatever they want in there.

@danieltabacaru
Copy link
Collaborator

danieltabacaru commented Feb 1, 2024

bindgen spec exposes unregister too. I know one can unregister by destroying the token (should be documented), so maybe that's good enough.

@nicola-cab
Copy link
Member Author

bindgen spec exposes unregister too. I know one can unregister by destroying the token (should be documented), so maybe that's good enough.

There is no C-API in the bingen (AFAIK), also all the tokens that we expose are destroyed via realm_release and that automatically stops the notifications. I think this is the general pattern we follow in the C-API.
Once we get to the point of generating a new C-API we can revisit this, I would say.

@danieltabacaru
Copy link
Collaborator

There is no C-API in the bingen (AFAIK)

I meant that you can unregister if you don't use the c-api, and I was thinking how well they both should align (I agree we have different patterns in the c-api, such as stopping notifications if the token is destroyed)

Copy link
Collaborator

@danieltabacaru danieltabacaru left a comment

Choose a reason for hiding this comment

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

LGTM

@nicola-cab nicola-cab merged commit d12c38d into master Feb 1, 2024
36 checks passed
@nicola-cab nicola-cab deleted the nc/expose_sync_user_subscribe branch February 1, 2024 12:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C-API] Expose SyncUser::subscribe()
2 participants