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

ffi: expose the notification settings #2223

Merged
merged 2 commits into from
Jul 7, 2023
Merged

Conversation

nimau
Copy link
Contributor

@nimau nimau commented Jul 5, 2023

This PR is the last step of the #1959 implementation.

It exposes the notification_settings crate to update notification settings for a room, and to define whether certain predefined push rules are enabled.

A NotificationSettingsDelegate can be defined to notify the client application each time push rules are updated.

@nimau nimau requested a review from a team as a code owner July 5, 2023 13:07
@nimau nimau requested review from poljar and removed request for a team July 5, 2023 13:07
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (afeb5da) 76.82% compared to head (31d66f2) 76.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2223   +/-   ##
=======================================
  Coverage   76.82%   76.82%           
=======================================
  Files         167      167           
  Lines       17748    17748           
=======================================
  Hits        13635    13635           
  Misses       4113     4113           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Looks good, the ULD needs a newline at the end and I believe that we can even skip modifying the UDL completely.


callback interface NotificationSettingsDelegate {
void settings_did_change();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at the end. @jplatte am I imagining things or can we now define callback interfaces via proc macros?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can :)

#[uniffi::export(callback_interface)]
trait NotificationSettingsDelegate { /* ... */ }

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, let's do that then. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice. Ok, I've updated the branch.

@nimau nimau requested a review from poljar July 7, 2023 12:30
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Looks good. If CI passes we can merge.

@poljar poljar merged commit fa4c1ef into main Jul 7, 2023
@poljar poljar deleted the nicolas/ffi_notification_settings branch July 7, 2023 13:07
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.

3 participants