[MRG] Bluetooth Feature for Form Exchanging #260

huangyz0918 · 2019-06-05T02:03:02Z

What has been done to verify that this works as intended?

I have tested it using my Huawei Nexus 6P (Android 8.0) and Meizu M5 (Android 6.0).

Why is this the best possible solution? Were any other approaches considered?

Now, we are going to build a bluetooth transfer feature. Using bluetooth, we can share form instances between two devices. For bluetooth BLE and bluetooth classic, I choose bluetooth classic because bluetooth classic is compatible with more devices (BLE can only supports device with Android Version >= 4.3).

I'm going to introduce the basic bluetooth tools for ODK Skunkworks-Crow in the PR, The basic tools can be divides into two parts, the bluetooth client and bluetooth server. For data transmission, we need one device act as a bluetooth client and another as a server. If one is not ready, the bluetooth transmission will failed. So I want to build an alert dialog to ask which method our user would like to use, the Wifi Hotspot? or the Bluetooth? Only two device choose the same approach, the connection can be established.

For building this new feature, we have several tasks,

build basic bluetooth transfer client method.
build basic bluetooth transfer server method.
build a new BluetoothActivity.java for showing scanning results and device list.
build data transfer methods (short messages, files).
rename files to make them more universal (e.g, HpSenderActivity.java and BtSenderActivity.java)
build the basic alert dialog to ask user to choose a transfer method.
build the sending/receiving forms (instance) feature.
improve the UI/UX progress.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It needs tests with new features and make sure the old features work well.

Before submitting this PR, please make sure you have:

run ./gradlew checkCode and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments and/or in the about file.

…with success.

built alert dialogs for choose different transfer approaches.

added alert dialog while connecting to other device; extracted the common methods to PermissionUtilies.java.

huangyz0918 · 2019-06-26T08:41:16Z

Now, I finished the bluetooth tools for file sending and receiving, I made a demo for sending a short message using bluetooth, you can see this video demo.

I created a bluetooth package in the root of this project,

BluetoothBasic.java: Basic methods for bluetooth connecting/sending/loop reading.
BluetoothReceiver.java: BroadcastReceiver for system bluetooth services.
BluetoothClient.java: extend from BluetoothBasic.java, for data sending.
BluetoothServer.java: extend from BluetoothBasic.java, for data receiving.
BluetoothUtils.java: common methods for bluetooth status checking, etc.

I built the whole bluetooth tools with data receiving/sending, but when I refer to the form instances, I saw there are a lot of code related to data processing in UploadJob.java and DownloadJob.java, so I'm confused: should I migrate the bluetooth sending/receiving code into the exist *Job.java files or build another BtUploadJob.java and extract some common method into utils like UploadUtils.java?

@shobhitagarwal1612 @lakshyagupta21 @Chromicle I think I need some suggestions for further updating, thanks.

huangyz0918 · 2019-06-30T15:44:55Z

@lakshyagupta21 @shobhitagarwal1612 I think it's ready for a review.

lakshyagupta21

Things looks good to me, but it needs some code changes related to style and some case handling please take a look.

skunkworks_crow/src/main/java/org/odk/share/services/ReceiverService.java

lakshyagupta21 · 2019-07-01T16:31:41Z

skunkworks_crow/src/main/java/org/odk/share/services/SenderService.java

        PersistableBundleCompat extras = new PersistableBundleCompat();
        extras.putLongArray(UploadJob.INSTANCES, instancesToSend);
+        extras.putBoolean("isBluetooth", false);


Same as above.

skunkworks_crow/src/main/java/org/odk/share/tasks/UploadJob.java

skunkworks_crow/src/main/java/org/odk/share/views/ui/send/fragment/FilledFormsFragment.java

lakshyagupta21 · 2019-07-01T17:06:12Z

skunkworks_crow/src/main/java/org/odk/share/views/ui/send/fragment/FilledFormsFragment.java

@@ -45,6 +49,7 @@
    public static final String INSTANCE_IDS = "instance_ids";
    private static final String INSTANCE_LIST_ACTIVITY_SORTING_ORDER = "instanceListActivitySortingOrder";
    private static final int INSTANCE_LOADER = 1;
+    private static final String[] TRANSFER_OPTIONS = {"via bluetooth", "via hotspot"};


Lets create or resuse the string resources and via can be removed from the string.

skunkworks_crow/src/main/java/org/odk/share/tasks/DownloadJob.java

lakshyagupta21 · 2019-07-01T17:49:42Z

skunkworks_crow/src/main/java/org/odk/share/tasks/UploadJob.java

            // show dialog and connected
            Timber.d("Start Sending");
            processSelectedFiles(instancesToSend);

            // close connection
-            socket.close();
-            serverSocket.close();
+            if (socket != null) {


When all the form transfer is done newly created bluetoothSocket also needs to closed for both sender and receiver other it may result in memory leaks.

Yes, thanks for reminding. Fixed.

lakshyagupta21 · 2019-07-01T17:52:29Z

skunkworks_crow/src/main/java/org/odk/share/views/ui/bluetooth/BluetoothListAdapter.java

+    /**
+     * rescan the bluetooth devices and update the list.
+     */
+    public void rescan() {


rescan logic should not be present in adpater class same for below method addBoundedDevices

See updated.

lakshyagupta21 · 2019-07-01T17:53:32Z

skunkworks_crow/src/main/java/org/odk/share/views/ui/bluetooth/BtReceiverActivity.java

+                .subscribe(bluetoothEvent -> {
+                    switch (bluetoothEvent.getStatus()) {
+                        case CONNECTED:
+                            progressDialog.setMessage("Connected, start downloading...");


Create string resource for this or use the same that is defined for hotspot.

lakshyagupta21 · 2019-07-01T17:55:54Z

Also one thing that I noticed while scanning the devices is that it is showing all the devices which was paired before to the device but not available at the moment of scanning, so please take a look at that as well.

shobhitagarwal1612 · 2019-07-01T18:09:54Z

skunkworks_crow/src/main/AndroidManifest.xml

@@ -18,6 +18,10 @@
    <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" />
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />

+    <!-- Bluetooth Related -->


The permissions are self explanatory. No need for this comment

shobhitagarwal1612 · 2019-07-01T18:17:47Z

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothReceiver.java

+        }
+    }
+
+    public interface Listener {


Please rename this interface to something less generic like BluetoothReceiverListener

shobhitagarwal1612 · 2019-07-01T18:19:31Z

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothReceiver.java

+    }
+
+    public interface Listener {
+        void foundDevice(BluetoothDevice device);


The name of the method should display its intention. So, onDeviceFound would sound more accurate. Comments?

Refactored.

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothUtils.java

shobhitagarwal1612 · 2019-07-01T18:23:09Z

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothUtils.java

+    /**
+     * Checking if current device supports bluetooth.
+     */
+    public static boolean isSupportBluetooth() {


Method name should be isBluetoothSupported

shobhitagarwal1612 · 2019-07-01T18:41:20Z

skunkworks_crow/src/main/java/org/odk/share/views/ui/send/fragment/BlankFormsFragment.java

-        Intent intent = new Intent(getActivity(), SendActivity.class);
+        Intent hotspotIntent = new Intent(getActivity(), HpSenderActivity.class);
+        Intent bluetoothIntent = new Intent(getActivity(), BtSenderActivity.class);
+        setupSendingIntent(hotspotIntent);


setup should happen after the TRANSFER_OPTION has been selected to prevent extra initialisation of the other intent.

shobhitagarwal1612 · 2019-07-01T18:42:00Z

skunkworks_crow/src/main/java/org/odk/share/views/ui/send/fragment/BlankFormsFragment.java

+                                startActivity(hotspotIntent);
+                                getActivity().finish();
+                            }
+                            break;


Switch cases have most of their code common. Please reuse the code.

skunkworks_crow/src/main/res/values/strings.xml

shobhitagarwal1612 · 2019-07-01T18:43:28Z

skunkworks_crow/src/main/res/layout/list_item_bt_device.xml

+            android:gravity="center_vertical"
+            android:textSize="12sp"
+            tools:text="default device address" />
+


Please remove blank line

shobhitagarwal1612 · 2019-07-01T18:43:35Z

skunkworks_crow/src/main/res/layout/list_item_bt_device.xml

+            tools:text="default device address" />
+
+    </LinearLayout>
+


same as above

huangyz0918 · 2019-07-04T15:58:33Z

@lakshyagupta21 @shobhitagarwal1612 I have improved the code as your suggested, please have a new round review and make sure the code is good to merge, thanks.

I have tested the new feature using sender/receiver with success:

Google Nexus 6P Android 8.0.0 (sender/receiver)
Google Nexus 6P Android 8.1.0 (sender/receiver)
Google Nexus 5 Android 6.0.1 (sender/receiver)
Meizu M5 Android 6.0 (sender/receiver)

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothReceiver.java

shobhitagarwal1612 · 2019-07-04T16:10:08Z

skunkworks_crow/src/main/java/org/odk/share/tasks/DownloadJob.java

-                if (bluetoothSocket.isConnected()) {
-                    rxEventBus.post(new BluetoothEvent(BluetoothEvent.Status.CONNECTED));
-                }
+                    if (bluetoothSocket.isConnected()) {


this can be replaced with else

I don't think this should go in else because previous if block says it to connect so I think this if stmt check can be removed and we can directly write
rxEventBus.post(new BluetoothEvent(BluetoothEvent.Status.CONNECTED));
as it is expected that

if (!bluetoothSocket.isConnected()) { bluetoothSocket.connect(); }

will ensure that bluetooth is connected and if by any chance it fails then it should throw some error or callback that we need to handle explicitly.

agreed with @lakshyagupta21 , just removing the if-else.

Is bluetoothSocket.connect(); a synchronous event? Are there any callbacks for bluetooth connected or disconnected? If not then we need to fix it.

Second problem is that we shouldn't establish bluetooth socket connection within DownloadJob. DownloadJob's purpose is to start download task and it shouldn't be doing anything else. This would testing the code in the later part difficult.

Is bluetoothSocket.connect(); a synchronous event? Are there any callbacks for bluetooth connected or disconnected? If not then we need to fix it.

This method is synchronous yes, and if the connection cannot be established, it will throw an IO Exception. So we don't have to fix that.

Second problem is that we shouldn't establish bluetooth socket connection within DownloadJob. DownloadJob's purpose is to start download task and it shouldn't be doing anything else. This would testing the code in the later part difficult.

An issue is, we cannot pass an object as a parameter into our jobs, I tried to establish the socket connection in the activity but it seems the job is not relay on that. Besides, the socket and stream for hotspot are created in the job as well, so I think it doesn't matter.

@shobhitagarwal1612 Can you suggest a forward, I tried the workarounds but I'm not able to think one because DownloadJob needs socket objects and we cannot pass the socket objects directly in the DownloadJob and UploadJob.

Okay. I'll give it a try and post it here.

huangyz0918 · 2019-07-09T13:22:52Z

I notice an issue when we are trying to send form instances, using two Google Nexus 6P (android 8.0.0 and android 8.0.1): The progress will stuck when we send form instance(s), I cannot use hotspot method to verify where this issue comes from (since wifi cannot be established in android 8.0.1, the hotspot cannot be turned on and the QR Code didn't show up either), so can you help me with that issue? @lakshyagupta21 @shobhitagarwal1612 I don't know if you have notice the android 8+ issue with hotspot, and I want to validate if the issue comes from here and fix that.

added some unit tests.

lakshyagupta21 · 2019-07-14T16:51:59Z

@huangyz0918 I'm still not sure about this PR whether it will break anything or not, and I need some more time to test it and I know that some of your tasks are dependent on this PR and @shobhitagarwal1612 is also thinking about solving the above issue. So we can merge this PR in some intermediate branch and then you can work considering that as your development branch. This way you'll get unblocked.
What do you say @shobhitagarwal1612 and @huangyz0918 ?

huangyz0918 · 2019-07-15T01:09:36Z

So we can merge this PR in some intermediate branch and then you can work considering that as your development branch.

Good idea to merge that into a dev branch. @lakshyagupta21 and I think you can have tests in the dev until you think it's ready for merging into master. How about @shobhitagarwal1612 , what do you think?

shobhitagarwal1612 · 2019-07-15T17:43:08Z

I think merging into a dev branch is not a good idea as this PR touches a lot of code that would create an extra overhead of keeping up that branch with the latest master branch. So, all of the subsequent PRs will still have lots of conflicts. So lets make sure that this PR does not disrupt the existing wifi-hotspot transfer functionality and merge this as it is. Then we can test this everyday and open issues as we find them.

…ed tests.

lakshyagupta21 · 2019-07-17T17:36:25Z

skunkworks_crow/src/test/java/org/odk/share/activities/BtReceiverActivityTest.java

+
+        boolean receiverFound = false;
+        for (ShadowApplication.Wrapper wrapper : registeredReceivers) {
+            if (!receiverFound) {


Its always going to be false first so whenever it gets inside the for each loop then the receiver will be assigned, then there is no use of iterating over registered receivers.
Anyways, the code requires clean up that we can do later.

lakshyagupta21 · 2019-07-17T18:01:37Z

@huangyz0918 @shobhitagarwal1612 Merging this PR, I did some testing on Bluetooth form transfer from Android 7 to Android 9, but I was not able to transfer the other way. And there were some issues related to UI/UX as well I think it is not in parity with the Hotspot functionality, like

before pressing back while form transfer it should show some dialog to get the confirmation for stopping the connection.
BlueTooth is still 'ON' even if the connection is complete/canceled.
I'm not able to connect to new bluetooth directly via an app and it doesn't show me any new device.
These are the basic issues which I think user will face, there can be more.

huangyz0918 · 2019-07-18T01:35:00Z

I'm not able to connect to new bluetooth directly via an app and it doesn't show me any new device.

I think you should wait longer since the bluetooth classic takes a very long time to discovery some other devices. I'm not sure how to improve that, any suggestions? @lakshyagupta21

huangyz0918 · 2019-07-18T01:36:11Z

BlueTooth is still 'ON' even if the connection is complete/canceled.

I can fix this, and I'll open an issue for that.

huangyz0918 added 11 commits June 4, 2019 20:50

✨ feat: add permissions in manifest.xml.

dfef497

✨ feat: added bluetooth utilities.

bf074e8

Merge remote-tracking branch 'upstream/master' into issue#253-bluetooth

82018dd

🚑 quickfix: fixed some pmd errors.

cff5a9e

🎨 style: fixed checkstyle errors.

887f5cd

✨ feat: build bluetooth activity and device adapter.

1427ff0

Merge remote-tracking branch 'upstream/master' into issue#253-bluetooth

6b2eda0

💄 ui: improved the attrs using style.xml.

0892e8a

✨ feat: built the server/client activity and test with transfer data …

b2f3b26

…with success.

🔨 refactor: refactor the package and built dialogs

99aec39

built alert dialogs for choose different transfer approaches.

🚑 quickfix: made bluetooth activities injectable.

3ab4393

huangyz0918 marked this pull request as ready for review June 24, 2019 14:16

shobhitagarwal1612 requested a review from lakshyagupta21 June 24, 2019 14:32

huangyz0918 changed the title ~~Bluetooth Utilities~~ [WIP] Bluetooth Utilities Jun 25, 2019

💄 ui: added alert dialog

4345919

added alert dialog while connecting to other device; extracted the common methods to PermissionUtilies.java.

huangyz0918 added 6 commits June 26, 2019 18:29

🚑 quickfix:

cc54980

✨ feat: finied the bluetooth transfer feature.

0f1f544

🚑 quickfix:

2d2c25b

🔨 refactor: simplified the code.

46f6402

✨ feat:

d3718f3

✨ feat:

4275ea7

huangyz0918 changed the title ~~[WIP] Bluetooth Utilities~~ [MRG] Bluetooth Feature for Form Exchanging Jun 30, 2019

lakshyagupta21 suggested changes Jul 1, 2019

View reviewed changes

shobhitagarwal1612 suggested changes Jul 1, 2019

View reviewed changes

huangyz0918 added 3 commits July 2, 2019 19:34

🚑 quickfix: fixed the single inten problem

92d1d06

🚑 quickfix:

84f01c8

🚑 quickfix(added methods option for review form mode.):

a33beb3

shobhitagarwal1612 reviewed Jul 4, 2019

View reviewed changes

skunkworks_crow/src/main/java/org/odk/share/bluetooth/BluetoothReceiver.java Outdated Show resolved Hide resolved

shobhitagarwal1612 reviewed Jul 4, 2019

View reviewed changes

🚑 quickfix: fixed code style and removed useless code.

36c35bd

shobhitagarwal1612 mentioned this pull request Jul 5, 2019

Add support for Android 9 #227

Open

2 tasks

This was referenced Jul 10, 2019

Write the unit tests for the bluetooth feature. #265

Closed

[Feature] Build the default transferring mechanism #266

Closed

huangyz0918 added 6 commits July 12, 2019 13:19

✅ test:

a924bb6

added some unit tests.

🚑 quickfix:

19044b4

🚑 quickfix:

c975c0f

🐛 fix: fixed the senderActivity launch a sync-task fialed the test.

067ff29

✅ test: added the tests for the bluetooth receiver.

f185d65

✅ test:

467f316

huangyz0918 added 4 commits July 16, 2019 10:10

✅ test: added the tests for AboutActivity.java

764afd4

✅ test: added tests for BtSenderActivity.java

cb17e80

✅ test: added the SendFormsActivity tests.

0801c08

🚑 quickfix: removed the BtSenderActivityTest since the sync task fail…

50ba51d

…ed tests.

lakshyagupta21 reviewed Jul 17, 2019

View reviewed changes

lakshyagupta21 approved these changes Jul 17, 2019

View reviewed changes

lakshyagupta21 merged commit be7cab3 into getodk:master Jul 17, 2019

This was referenced Jul 18, 2019

Bluetooth is still 'ON' even if the connection is complete/canceled. #268

Closed

Show dialog to get the confirmation for stopping the bluetooth connection. #269

Closed

Java.io.IOException: Socket Closed #267

Closed

[MRG] Bluetooth Feature for Form Exchanging #260

[MRG] Bluetooth Feature for Form Exchanging #260

Conversation

huangyz0918 commented Jun 5, 2019 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Before submitting this PR, please make sure you have:

huangyz0918 commented Jun 26, 2019 • edited Loading

huangyz0918 commented Jun 30, 2019

lakshyagupta21 left a 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

lakshyagupta21 commented Jul 1, 2019

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

huangyz0918 commented Jul 4, 2019

Choose a reason for hiding this comment

lakshyagupta21 Jul 4, 2019 • 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

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

huangyz0918 commented Jul 9, 2019

lakshyagupta21 commented Jul 14, 2019

huangyz0918 commented Jul 15, 2019

shobhitagarwal1612 commented Jul 15, 2019

Choose a reason for hiding this comment

lakshyagupta21 commented Jul 17, 2019

huangyz0918 commented Jul 18, 2019

huangyz0918 commented Jul 18, 2019 • edited Loading

huangyz0918 commented Jun 5, 2019 •

edited

Loading

huangyz0918 commented Jun 26, 2019 •

edited

Loading

lakshyagupta21 Jul 4, 2019 •

edited

Loading

huangyz0918 commented Jul 18, 2019 •

edited

Loading