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

Backport of NotificationBucket #4550

Merged
merged 3 commits into from
May 3, 2022
Merged

Backport of NotificationBucket #4550

merged 3 commits into from
May 3, 2022

Conversation

kraenhansen
Copy link
Member

What, How & Why?

This is a backport of #4228 onto master, in an attempt to ease the rebase of v11 once #4522 is merged to master and released.

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

@kraenhansen kraenhansen self-assigned this May 3, 2022
@cla-bot cla-bot bot added the cla: yes label May 3, 2022
CHANGELOG.md Outdated Show resolved Hide resolved
src/js_notifications.hpp Outdated Show resolved Hide resolved
src/js_notifications.hpp Outdated Show resolved Hide resolved
src/js_notifications.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fronck fronck left a comment

Choose a reason for hiding this comment

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

I left a few suggestions; otherwise LGTM.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

Found a typo and have a question. LGTM

src/js_notifications.hpp Outdated Show resolved Hide resolved
*/
inline static TokensMap& get_tokens()
{
static TokensMap s_tokens;
Copy link
Contributor

Choose a reason for hiding this comment

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

[question] I just am curious what s_ indicates? (Probably something about it being static). I would also like to know how this does get initialised. Does static in this case behave a bit like a singleton?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably something about it being static.

Right. I personally don't like this convention, but I believe it got introduced in the previous implementation because other pieces of the code adhered to it.

I would also like to know how this does get initialised.

This is the initialisation - this syntax calls the default constructor with no arguments.

Co-authored-by: FFranck <[email protected]>
Co-authored-by: Andrew Meyer <[email protected]>
@cla-bot cla-bot bot added the cla: yes label May 3, 2022
@kraenhansen kraenhansen merged commit 6140d46 into master May 3, 2022
@kraenhansen kraenhansen deleted the kh/notification-buckets branch May 3, 2022 11:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants