Details

Reviewers

tomek
• karol

Commits

rCOMM3937a61faf82: Use MessageOperationsUtilities on Android

Summary

This diff uses MessageOperationsFunctionality and JNI files from previous diff to store notification in CommNotificationsHandler on Android.

Test Plan

Log message content in C++ when sending notification. Do this for every message type. Additionally temporarily modify send.js prepareAndroidNotification code to send notification as if codeVersion < 31 and check that messages are still parsed correctly.

Comment out set_encryption_key method in SQLiteQueryExecutor (to disable encryption) code build app on an emulator, launch it and immediately kill. Then send a message from another client. Open device file explorer in Android studio and download comm.sqlite database. Examine its content in some sqlite explorer (vscode has a preety good one) to ensure that messages are there even though app was not launched after sending message.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.May 18 2022, 3:35 AM

Herald added subscribers: • abosh, atul, • adrian, ashoat. · View Herald TranscriptMay 18 2022, 3:35 AM

marcin added a parent revision: D4065: Implement JNI files to use MessageOperationUtilities in CommNotificationsHandler on Android.May 18 2022, 3:36 AM

Harbormaster completed remote builds in B9259: Diff 12856.May 18 2022, 3:42 AM

marcin requested review of this revision.May 18 2022, 3:42 AM

Rebase

Harbormaster failed remote builds in B9275: Diff 12872!May 18 2022, 4:13 AM

Rebase

Harbormaster completed remote builds in B9287: Diff 12887.May 18 2022, 5:12 AM

Harbormaster completed remote builds in B9275: Diff 12872.May 18 2022, 7:07 AM

tomek requested changes to this revision.May 18 2022, 9:58 AM

tomek added inline comments.

native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java
103–104 ↗	(On Diff #12887)	We should inverse the order here. It's probably cheaper to first check if the notification has message infos than to access the file system. So check the file system only after we're sure we have something to save.
123–124 ↗	(On Diff #12887)	That's probably fine - it may happen

This revision now requires changes to proceed.May 18 2022, 9:58 AM

Change validation order

Harbormaster completed remote builds in B9337: Diff 12938.May 19 2022, 4:57 AM

marcin edited the test plan for this revision. (Show Details)May 19 2022, 5:52 AM

tomek requested changes to this revision.May 20 2022, 1:47 AM

tomek added inline comments.

native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java
105–112 ↗	(On Diff #12938)	We don't need this code at all. `codeVersion` indicates the client version of code. If we have a client with code version < 31, and that client would like to have the code you implemented, an update should be made which will increase the code version. Older code versions should always be handled on server side and never on client side.

This revision now requires changes to proceed.May 20 2022, 1:47 AM

Remove unnecessary code that parses < 31 version notification structure.

Harbormaster completed remote builds in B9367: Diff 12982.May 20 2022, 8:11 AM

tomek requested changes to this revision.May 20 2022, 8:23 AM

tomek added inline comments.

native/android/app/src/main/java/app/comm/android/CommNotificationsHandler.java
103 ↗	(On Diff #12982)	This name is misleading as it is a `File` and we name it `path`. Maybe just `sqliteFile` would be ok?
106 ↗	(On Diff #12982)	We still need to update the name, right?
107 ↗	(On Diff #12982)	In Java we should avoid using `toString` for any logic - it is ok for debugging purposes but nothing more. It looks like `File` has `getPath` method - should we use that?