Updates thread unread status in response to rescind on Android.
Details
Details
Comment-out encryption code in SQLiteQuery executor. Create two clients - A (web), B(mobile and web). Send message from A to B while mobile B is inactive. Open mobile B but do not read the message. Download the database from Android Studio. Kill mobile B, read message on web B and download SQLite database from Android Studio. Check that unread status in a thread the message was sent for changed from true to false.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java | ||
|---|---|---|
| 54–55 ↗ | (On Diff #13393) | Technically not since every time it is called it replaces SecureStoreModule instance that is used in CommSecureStore. In practice SecureStoreModule is a stateless module so it does not affect functionality. But I do understand it might be safe to modify CommSecureStore.getInstance().initialize() to be idempotent. It should not require lot of work - should I implement it? |
| 66–86 ↗ | (On Diff #13393) | Generally through my experience I was advised not to extract some logic into a function if it is not called at least twice. On the other hand any kind of logic we implement in this NotificationService is unlikely to be called twice, so I think it might be a good idea to extract it into a private method since in future we might end up with one huge piece of code inside onMessageReceived callback that might be hard to maintain. |
| native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java | ||
|---|---|---|
| 54–55 ↗ | (On Diff #13393) | Hmmm... not entirely sure, but here's my thinking:
|
| 66–86 ↗ | (On Diff #13393) | I think it can be good for making the code more self-documenting, and for reducing the length of this file. @palys-swm, what do you think? |
| native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java | ||
|---|---|---|
| 54–55 ↗ | (On Diff #13393) | Using Java AtomicBoolean built-in feature can make this call idempotent in a thread safe way using one if statement. |
| 54–55 ↗ | (On Diff #13393) | At this moment I think it is short, so I will prepare a diff and put it before this one in the stack. If during the review process it appears that my solution is not correct and the problem requires more complex approach, I will create Linear task. |
| native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java | ||
|---|---|---|
| 54–55 ↗ | (On Diff #13393) | I'm not sure if it is that easy, e.g. if we have 2 threads (A and B), if A sets atomic value to true and then wants to initialize and set value, B could check the atomic value before A finished writing - it will cause a NullPointerException. By far the best solution to initialize is to avoid doing it lazily and instead use static variable. But in this case it might be impossible, because we need to get the application context. The proper solution to this issue is to use double-check idom https://medium.com/codex/effective-java-use-lazy-initialization-judiciously-96cfa1dc288d |
| 66–86 ↗ | (On Diff #13393) |
This wasn't a good advice. Extracting to a function can reduce the duplication, but it also helps with documentation (good function name describes what the function does), organization of code, separation of concerns, etc. - so it improves readability and maintainability. So in this case, it might be a good idea to have a separate function e.g. for every type of notification. |
| native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java | ||
|---|---|---|
| 54–55 ↗ | (On Diff #13393) | This is close, but we still might end up with creating a couple of SecureStoreModule and ignore all but the one that we assign. This isn't going to cause any harm, but is a little wasteful. A possible solution might be to modify initialize method of CommSecureStore so that it takes a Supplier<SecureStoreModule> (which is a function that takes no args and returns SecureStoreModule) and call this only when we need to assign. CommSecureStore.getInstance().initialize(
() -> new SecureStoreModule(this.getApplicationContext())); |