-
Notifications
You must be signed in to change notification settings - Fork 73
Add support for Android 9 #227
base: master
Are you sure you want to change the base?
Conversation
I've requested a review from people I remember might have mentioned that they have an Android 9 device to try this PR out. If I forgot someone's name, then apologies for that. |
@shobhitagarwal1612 i don't have a android 9 device now but i can get my hands on one by monday |
@shobhitagarwal1612 Please fix the conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shobhitagarwal1612 It is working fine for me now and close the support for Android 8.0.0 when this is merging, I just now checked it is working fine as expected
Thanks for testing it out @Chromicle and I'm glad that it worked for you. Could you please mention which all devices you used for testing this PR? It would be nice to keep a track of devices and android versions |
42223c6
to
d0c1793
Compare
Samsung J8 of Android 8.0.0 and samsung note 3 of Android 9 these all devices I tested it is working fine |
d0c1793
to
1fafb0c
Compare
@lakshyagupta21 Done |
@shobhitagarwal1612 i just tested it in samsung j7(2016) - android 8.1. I noticed that AlertDialog keeps popping one after the another until the app becomes un-responsive |
1fafb0c
to
a8b82bd
Compare
I found another bug, not the one you have mentioned @udhay24. Also, I tested on Android 9 as I don't have an Android 8 device |
@udhay24 I just now checked I did not find that type of behavior in android 8.0.0 |
I didn't find a way to get the status of the hotspot in Android 9. Hence the alert dialog. My intention behind this PR was to give a push to this GSoC project and get the base ready i.e. sending/receiving so that you guys can focus on new features and overall stability. All these issues can be opened separately and fixed during the summer |
@shobhitagarwal1612 i forgot to mention that the location was already enabled but the app was still asking to enable the location. |
@udhay24 I was able to reproduce the issue. Please review again |
Add a helper class for shared preferences
Remove localOnlyHotspot, turns out server socket doesn't work on it. I tried many alternatives but none seem to work. The connection is unreachable no matter what over localOnlyHotspot. So, the only way out seems to be using system enabled hotspot triggered manually by user
This is a regression of updating sdk version to 28, Needs to fixed separately
bbeff6d
to
9e8fc60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fix for android 9.0, I have tested it using Nexus 6P and Pixel XL and they both worked!
Several improvements for your reference.
return ssid.contains(getString(R.string.hotspot_name_suffix)) || | ||
ssid.contains(getString(R.string.hotspot_name_prefix_oreo)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the code is useless, please remove them or making comments to clarify the reason for keeping them.
port = (Integer) obj.get(PORT); | ||
isProtected = (boolean) obj.get(PROTECTED); | ||
if (isProtected) { | ||
passwordScanned = (String) obj.get(PASSWORD); | ||
} | ||
|
||
Timber.d("Scanned results " + ssidScanned + " " + port + " " + isProtected + " " + passwordScanned); | ||
Timber.d("Scanned results " + wifiNetworkSSID + " " + port + " " + isProtected + " " + passwordScanned); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing all the test log if they are only used for debugging, or just comment them.
@@ -397,19 +399,25 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) { | |||
// request was canceled... | |||
Timber.i("QR code scanning cancelled"); | |||
} else { | |||
Timber.d("RESULT " + result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above.
} else { | ||
Toast.makeText(this, "already connected to " + ssidScanned, Toast.LENGTH_SHORT).show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the toasts into something like ToastUtils.java
? You can refer to Collect's. Something for showing dialogs can be refactor as well.
@@ -106,11 +102,10 @@ protected void onCreate(Bundle savedInstanceState) { | |||
formIds = getIntent().getLongArrayExtra(FORM_IDS); | |||
} | |||
|
|||
|
|||
port = SocketUtils.getPort(); | |||
|
|||
if (port == -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
can be replaced by a static variable with meaningful name, like NO_AVAILABLE_PORT_CODE
?
isHotspotInitiated = true; | ||
|
||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
// do nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//TODO:
will be helpful.
alertDialog.setCancelable(false); | ||
alertDialog.setButton(DialogInterface.BUTTON_POSITIVE, getString(R.string.ok), quitListener); | ||
alertDialog.show(); | ||
new AlertDialog.Builder(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So many similar dialogs, can we building something like DailogFactory.java
which has different static methods can return dialogs?
} | ||
|
||
if (openSettings) { | ||
} else if (openSettings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openSettings
is a bad name for a boolean value.
} | ||
|
||
public void stopHotspot() { | ||
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those version checks, we can build a boolean variable like isApiHigherThanO = ...
, can call isApiHigherThanO
or !isApiHigherThanO
if necessary, which can simplify the code and make them better to read.
@lakshyagupta21 What are your thoughts about this PR? Do you think this would be a good solution to some of the mentioned wifi hotspot issues? |
Even I think that should resolve some of the issues. yeah it would be a good idea to let @huangyz0918 pick this up, if he needs some help with the Android 9 testing in case he doesn't have one then we can hop in. |
Yeah I'd like to, but I don't have two android 9 devices currently, so I may have some trouble testing the changes. Can you help me to test the PR #260 using android 9 first? @lakshyagupta21 I want to know what kinds of issue in android 9 with bluetooth feature (I tested the PR in android 8.0.0, 8.0.1 and 6.0.1 with success), for you guys have android 9 devices @Chromicle @udhay24 @iadeelzafar , I think it will be very nice to have a quick test with that too, thanks so much!! |
You don't need to have both android 9 devices at the same time. The same device can be used for validating sending/receiving one at a time. |
I think I can flash my Nexus 6P to android 9 after #260 was merged. If there are issues with android 9, we can open another issue for that. |
@@ -88,9 +87,6 @@ | |||
private long[] formIds; | |||
private int mode; | |||
|
|||
private WifiManager.LocalOnlyHotspotReservation hotspotReservation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shobhitagarwal1612 Is there any specific reason for removing this class and its API usage? I think Google has introduced this Class and some APIs so that hotspot connection can be made and two nearby devices can share the data locally without sharing the user's mobile data.
Closes #191
What has been done to verify that this works as intended?
Tested by sending forms multiple times on MotoG5 Plus (Android 9)
Why is this the best possible solution? Were any other approaches considered?
Not the best solution but this is the best I could come up with. Please suggest if you have any other approaches. So the current approach goes like this:
startLocalOnlyHotspot
as server socket wasn't reachable with that way. It works perfectly to start/stop hotspot but the ultimate goal wasn't being served. I searched a lot but wasn't able to find anything solid as to why the sockets were unreachable. Although, the host was pingable (tested byping <host_ip>
from the shell of the receiver device)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?
Need feedback for this approach. Might still contain bugs but at least it works now.
If you are testing this PR, then uninstall the currently installed app. Need shared preferences to be cleared.
Before submitting this PR, please make sure you have:
./gradlew checkCode
and confirmed all checks still pass OR confirm CircleCI build passes