Details

Reviewers

tomek
bartek

Commits

rCOMM9d9a441f5ea3: Add getter and setter for integers, getter for all keys and function to remove…

Summary

This differential extends MMKV API with integer getter and setter, function to retrieve all keys in the db and function to remove subset of keys from the db.

Test Plan

iOS:

Below the comm::CommMMKV::initialize() line add lines that set several different integers under different keys.
In NotificationService.mm at the beginning add lines that:
1. Get all keys.
2. Tries to get value for each key.
Then call remove with one-element array with one key.
Get all keys again and log them.
Send notification and check that all integers are logged and the set of keys logged doesn't contain the one passed to removeKeys.

Android:

Below the CommMMKV.initialize() line add lines that set several different integers under different keys.
In CommNotificationsHandler.java at the beginning add lines that:
1. Get all keys.
2. Tries to get value for each key.
Then call remove with one-element array with one key.
Get all keys again and log them.
Send notification and check that all integers are logged and the set of keys logged doesn't contain the one passed to removeKeys.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Feb 12 2024, 9:12 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 12 2024, 9:12 AM

marcin added a parent revision: D11046: Introduce MMKV to Android app.Feb 12 2024, 9:13 AM

Harbormaster completed remote builds in B26769: Diff 37009.Feb 12 2024, 9:26 AM

marcin requested review of this revision.Feb 12 2024, 9:26 AM

marcin edited the test plan for this revision. (Show Details)Feb 12 2024, 9:48 AM

marcin added inline comments.Feb 12 2024, 9:52 AM

native/cpp/CommonCpp/Tools/CommMMKV.h
17 ↗	(On Diff #37009)	MMKV API can't return null when we try to get integer that doesn't exist. It allows us to set default value that is returned instead in case the integer isn't present. So my idea here is that the developer should pass as `noValue` the value that they believe should never be set under certain key. For example if we use this for unreadCount we could pass -1 since unread count is never negative. Our implementation passes this as default value and if MMKV returns default value then our implementation returns nullish integer (`null` in Java and `std::nullopt` in C++ and Objective-C).

Add method to remove subset of keys

marcin retitled this revision from Add getter and setter for integers and getter for all keys to MMKV to Add getter and setter for integers, getter for all keys and function to remove subset of keys to MMKV.Feb 13 2024, 1:45 AM

marcin edited the summary of this revision. (Show Details)

marcin edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B26780: Diff 37020.Feb 13 2024, 1:57 AM

tomek accepted this revision.Feb 14 2024, 8:55 AM

tomek added inline comments.

native/android/app/src/cpp/CommMMKV.cpp
83 ↗	(On Diff #37020)	What is the result of `make_jstring`? Are we dereferencing a pointer? Should we free the memory at the end?
85 ↗	(On Diff #37020)	Why can the `setString` method accept `std::string` but `removeKeys` need to transform the parameters using `make_jstring`?
native/cpp/CommonCpp/Tools/CommMMKV.h
17 ↗	(On Diff #37009)	Can we explain it in a code comment?

This revision is now accepted and ready to land.Feb 14 2024, 8:55 AM

marcin added a child revision: D11071: Implement JSI to handle unread count from JS.Feb 15 2024, 5:29 AM

Update iOS code to fix reentrancy issues with the NSE

Harbormaster completed remote builds in B27248: Diff 37723.Mar 1 2024, 8:09 AM

Refactor to match changes in parent differential

Harbormaster completed remote builds in B27250: Diff 37727.Mar 1 2024, 9:29 AM

marcin added inline comments.Mar 6 2024, 6:47 AM

native/android/app/src/cpp/CommMMKV.cpp
83 ↗	(On Diff #37020)	What is the result of make_jstring? Calling `make_jstring` returns `local_ref.`. See here. Are we dereferencing a pointer? Should we free the memory at the end? The following fbjni doc explains that underlying objects are destructed when reference is destructed. Additionally this docs states explicitly that `local_ref` follows reference counting destruction paradigm.
85 ↗	(On Diff #37020)	Why can the setString method accept std::string This fbjni doc states explicitely that C++ methods calling Java can work with both `std::string` and `JString`. but removeKeys need to transform the parameters There is no similar conversion from `std::vector<T>` to `JArrayClass<T>` unfortunately. Working with arrays is explained here. using `make_jstring`? The `setElement` method of `JArrayClass<T>` expects that we are setting an object that is plain JNI reference.