QR code scanning improvements and PR feedback #314

oxtoacart · 2021-08-17T14:02:53Z

If you refactored existing code, have you tested the refactored functionality against the old version to make sure you didn't break anything?
Do the tests pass? Consistently?
Did this change improve test coverage?
Is the code in question being linted? If not, consider adding a linter step to CI. If yes, make sure the linter is happy.
Have you logged tickets for related technical debt with the label “techdebt”?

oxtoacart · 2021-08-17T14:15:36Z

android/app/build.gradle

@@ -417,7 +417,7 @@ dependencies {
    implementation(name:'opuslib-release', ext: 'aar')
    implementation 'com.github.getlantern:secrets-android:3ac588e'
    implementation 'com.github.getlantern:db-android:4d13d81641'
-    implementation 'com.github.getlantern:messaging-android:6c54447c7a'
+    implementation 'com.github.getlantern:messaging-android:fa0be640a1'


This pulls in changes from this PR, which includes both QR code scanning improvements as well as fixes related to introductions.

oxtoacart · 2021-08-17T14:16:07Z

android/app/src/main/kotlin/io/lantern/android/model/MessagingModel.kt

-            "addOrUpdateDirectContact" -> messaging.addOrUpdateDirectContact(
-                call.argument("identityKey")!!,
-                call.argument("displayName")!!
+            "addProvisionalContact" -> messaging.addProvisionalContact(


When scanning QR codes, we now add provisional contacts rather than full contacts. That keeps us from getting contacts before the other side has completed their part of the scanning dance.

oxtoacart · 2021-08-17T14:16:57Z

lib/messaging/conversation.dart

        : unawaited(model.clearCurrentConversationContact());
    return WillPopScope(
      onWillPop: () => Future<bool>.value(_keyboardState),
-      child: BaseScreen(
+      child: model.singleContactById(context, widget._contactId,


We need to subscribe to the contact so that we can observe changes that impact our conversation sticker (e.g. change to field hasReceivedMessage).

oxtoacart · 2021-08-17T14:19:51Z

lib/messaging/conversation.dart

-            .toList()
-            .where((intro) => intro.value.contactId == contact.contactId)
-            .isNotEmpty;
+        final isPendingIntroduction = !contact.hasReceivedMessage &&


An introduction is really only pending until we've received a message from the contact. This new hasReceivedMessage field let's us know once we've gotten anything (even just a control message like disappearSettings), which indicates to us that the other side must have added us as a contact.

oxtoacart · 2021-08-17T14:20:19Z

lib/messaging/conversation.dart

+            introductions
+                .toList()
+                .where(
+                    (intro) => intro.value.introduction.to == contact.contactId)


This filter was wrong. We're interested in introductions made to this conversation's contact, not those made by this conversations' contact.

oxtoacart · 2021-08-17T14:20:30Z

lib/messaging/conversation.dart

        return ConversationSticker(
-            contact: contact,
-            noIntroductions: noIntroductions,


I don't think noIntroductions is necessary.

oxtoacart · 2021-08-17T14:20:57Z

lib/messaging/introduce.dart

@@ -45,7 +45,7 @@ class _IntroduceState extends State<Introduce> {
                    padding: const EdgeInsets.symmetric(
                        horizontal: 24.0, vertical: 16.0),
                    child: Text(
-                        'Select two or more contacts to introduce.  They will be sent invitations to start messaging each other. '
+                        'Select two or more contacts to introduce.  They will be sent introductions to start messaging each other. '


"Invitation" means something different than "Introduction". Let's use "Introduction" consistently to avoid confusion.

oxtoacart · 2021-08-17T14:21:18Z

lib/messaging/introduce.dart

@@ -102,7 +102,7 @@ class _IntroduceState extends State<Introduce> {
                                            }),
                                          )),
                                ),
-                                (selectedContactIds.isNotEmpty)
+                                (selectedContactIds.length >= 2)


We should only display the button if we've selected 2 or more contacts, since we need at least 2 to make an introduction.

oxtoacart · 2021-08-17T14:21:29Z

lib/messaging/introductions.dart

@@ -1,4 +1,3 @@
-import 'package:lantern/core/router/router.gr.dart';


Linter drive-by

oxtoacart · 2021-08-17T14:22:10Z

lib/messaging/widgets/contacts/add_contact_QR.dart

@@ -20,7 +20,8 @@ class _AddViaQRState extends State<AddViaQR> {
  QRViewController? qrController;

  bool scanning = false;
-  Contact? scannedContact;
+  String? scannedContactId;


Changes in this file are to accommodate the switch to using provisional contacts for QR code scanning.

oxtoacart · 2021-08-17T14:26:27Z

lib/messaging/widgets/contacts/add_contact_username.dart

@@ -21,8 +20,6 @@ class _AddViaUsernameState extends State<AddViaUsername> {

  @override
  Widget build(BuildContext context) {
-    var model = context.watch<MessagingModel>();


Drive-by, unused

oxtoacart · 2021-08-17T14:26:59Z

lib/messaging/widgets/conversation_components/conversation_sticker.dart

-                ? _fullyAddedIcon()
-                : _partiallyAddedIcon(),
+        child:
+            !isPendingIntroduction ? _fullyAddedIcon() : _partiallyAddedIcon(),


This logic was backwards. We should only show the fully added state if we're NOT pending introduction.

oxtoacart · 2021-08-17T14:27:29Z

lib/messaging/widgets/message_types/contact_connection_card.dart

@@ -62,45 +61,39 @@ class ContactConnectionCard extends StatelessWidget {
                              color: outbound
                                  ? outboundMsgColor
                                  : inboundMsgColor),
-                        IconButton(


Having only the tiny icon as a click target seemed inconvenient, so I changed this to make the whole message bubble clickable.

oxtoacart · 2021-08-17T14:27:49Z

lib/messaging/widgets/message_types/contact_connection_card.dart

@@ -146,7 +139,7 @@ class ContactConnectionCard extends StatelessWidget {
                        dense: true,
                        leading:
                            const Icon(Icons.check_circle, color: Colors.black),
-                        title: Text('Accept'.i18n),
+                        title: Text('Accept'.i18n, style: tsAlertDialogTitle),


This needed the style to look the same as the Reject button.

oxtoacart · 2021-08-17T14:28:02Z

lib/messaging/widgets/message_types/contact_connection_card.dart

@@ -161,6 +154,7 @@ class ContactConnectionCard extends StatelessWidget {
                                icon: ImagePaths.alert_icon,
                                buttonText: 'OK'.i18n);
                          } finally {
+                            await context.router.pop();


For now, let's just close the bottom sheet until we figure out what else to do.

oxtoacart · 2021-08-17T14:28:09Z

lib/messaging/widgets/voice_recorder/audio_widget.dart

@@ -66,12 +66,6 @@ class AudioController extends ValueNotifier<AudioValue> {
    });
  }

-  void _setThumbnail(Uint8List thumbnail) {


Drive-by, unused

oxtoacart · 2021-08-17T14:28:17Z

lib/model/lru_cache.dart

- * [error] if loading fails, this contains the error encountered
- *
- */
+/// An asynchronously loaded value cached in an LRU cache. CachedValues are


Drive-by, comment style

cosmicespresso

Thank you for the bug fixes @oxtoacart 👍

In terms of the QR side of this - I am getting an error when I exit the scanning view without the other phone having scanned my code.

I/GoLog   (28038): Aug 17 15:19:21.860 - 1m41s ERROR lantern: logger.go:8 BaseModel: Unexpected error calling deleteProvisionalContact: java.lang.NullPointerException [error=%s: %s error_location=github.com/getlantern/android-lantern/internalsdk.Error (logger.go:8) error_text=BaseModel: Unexpected error calling deleteProvisionalContact: java.lang.NullPointerException error_type=errors.Error]
I/GoLog   (28038): ERROR lantern: logger.go:8   at github.com/getlantern/android-lantern/internalsdk.Error (logger.go:8)
I/GoLog   (28038): ERROR lantern: logger.go:8   at main.proxyinternalsdk__Error (go_internalsdkmain.go:1434)
I/GoLog   (28038): ERROR lantern: logger.go:8   at _cgoexp_6a4be5f45433_proxyinternalsdk__Error (_cgo_gotypes.go:1647)
I/GoLog   (28038): ERROR lantern: logger.go:8   at runtime.cgocallbackg1 (cgocall.go:292)
I/GoLog   (28038): ERROR lantern: logger.go:8   at runtime.cgocallbackg (cgocall.go:228)
I/GoLog   (28038): ERROR lantern: logger.go:8   at runtime.cgocallback (asm_arm64.s:1055)
I/GoLog   (28038): ERROR lantern: logger.go:8   at runtime.goexit (asm_arm64.s:1130)

oxtoacart · 2021-08-17T15:44:31Z

In terms of the QR side of this - I am getting an error when I exit the scanning view without the other phone having scanned my code.

Oops! I've fixed that now.

cosmicespresso · 2021-08-17T16:03:14Z

lib/messaging/widgets/contacts/add_contact_QR.dart

    subscription = qrController?.scannedDataStream.listen((scanData) async {
      try {
-        if (scannedContact != null) {
+        if (scannedContactId != null) {


should we be providing some UX feedback here? (sorry, should have pointed this out way earlier!)

No, that's just a guard because we can get the same event multiple times prior to shutting off the camera.

cosmicespresso · 2021-08-17T16:13:33Z

lib/messaging/widgets/contacts/add_contact_QR.dart

+          // immediately invoke listener in case the contactNotifier already has
+          // a contact.
+          listener();
+        }
      } catch (e) {


I'm getting a "Failed assertion: boolean expression must not be null" error when scanning the QR code of an existing contact in the catch statement here.

Not seeing this anymore @oxtoacart, I was one commit behind when I added it.

cosmicespresso · 2021-08-18T09:44:22Z

This works well for me now @oxtoacart, not getting that error after exiting QR screen.

oxtoacart added 4 commits August 16, 2021 22:40

Integrated bidirectional provisional contacts for QR code scanning

551f504

Introduction tweaks

bf27701

Code-review tweaks

2425391

Merged from kr/connectFriends

cb2b179

oxtoacart requested a review from cosmicespresso August 17, 2021 14:14

oxtoacart commented Aug 17, 2021

View reviewed changes

Removed unnecessary noIntroductions value

12d23d0

oxtoacart commented Aug 17, 2021

View reviewed changes

lib/messaging/introductions.dart

@@ -1,4 +1,3 @@

import 'package:lantern/core/router/router.gr.dart';

Copy link

Author

oxtoacart Aug 17, 2021

Choose a reason for hiding this comment

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

Linter drive-by

oxtoacart commented Aug 17, 2021

View reviewed changes

Delete provisional contact when exiting QR code scanning view

ca76562

oxtoacart commented Aug 17, 2021

View reviewed changes

oxtoacart added 2 commits August 17, 2021 10:06

Fixed subscription of single contact value

8b3d63e

Added TODO comment about QR code scanning after deletion of a contact

5beae9d

cosmicespresso reviewed Aug 17, 2021

View reviewed changes

More QR code scanning fixes

2788b9f

cosmicespresso reviewed Aug 17, 2021

View reviewed changes

Using mostRecentHelloTs to make support re-adding of existing contacts

9bc670c

cosmicespresso reviewed Aug 17, 2021

View reviewed changes

Merged from kr/connectFriends

7a8550d

oxtoacart force-pushed the ox/bidi-invite branch from 7851e6b to 7a8550d Compare August 18, 2021 13:04

oxtoacart merged commit 6166009 into kr/connectFriends Aug 18, 2021

oxtoacart deleted the ox/bidi-invite branch August 18, 2021 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QR code scanning improvements and PR feedback #314

QR code scanning improvements and PR feedback #314

oxtoacart commented Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

oxtoacart Aug 17, 2021

cosmicespresso left a comment

oxtoacart commented Aug 17, 2021

cosmicespresso Aug 17, 2021 •

edited

Loading

oxtoacart Aug 17, 2021

cosmicespresso Aug 17, 2021

cosmicespresso Aug 17, 2021

cosmicespresso commented Aug 18, 2021

		@@ -1,4 +1,3 @@
		import 'package:lantern/core/router/router.gr.dart';

QR code scanning improvements and PR feedback #314

QR code scanning improvements and PR feedback #314

Conversation

oxtoacart commented Aug 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicespresso left a comment

Choose a reason for hiding this comment

oxtoacart commented Aug 17, 2021

cosmicespresso Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicespresso commented Aug 18, 2021

cosmicespresso Aug 17, 2021 •

edited

Loading