-
Notifications
You must be signed in to change notification settings - Fork 497
GH-480: Implement Wifi signal strength #616
GH-480: Implement Wifi signal strength #616
Conversation
Samples are available, I will further test this code tomorrow. It is still not quite complete and thus should not yet be reviewed. |
This comment has been minimized.
This comment has been minimized.
static SignalStrength PlatformSignalStrength() | ||
{ | ||
var wifiManager = Platform.WifiManager; | ||
if (wifiManager is null) |
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.
This is redundant as you do wifiManager?. below so you can remove 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.
Acutally I would rather remove the ?. as this check is necessary to return unknown value if we do not have a wifi Manager instance available if that were acceptable
This comment has been minimized.
This comment has been minimized.
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.
This is a really nice PR, and I think it is quite clean. I do have questions about the signal values (see my comments). Other that the translation of platform values to the shared values, I think we have a nice one here.
I see you said that you are still working on this PR and should not be reviewed 😉 But we are all eager beavers here 😁 I hope our comments are helpful. Thanks for taking the time to work on this! |
This comment has been minimized.
This comment has been minimized.
@mattleibow Thank you for taking time to already look at this :) Yes, they were very helpful thank you :) |
I got around to testing android and uwp where this works just fine for me. Since I recently switched jobs I did not set up a developer account for iOS yet. If anyone would like to verify that it works that would be great. Otherwise I would love further discussion of the unresolved comments :) |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
I see @Redth discovered the suggestions feature ;) |
Co-Authored-By: Mrnikbobjeff <schillinik@yahoo.de>
❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
Co-Authored-By: Mrnikbobjeff <schillinik@yahoo.de>
❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
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 think this looks good!
@@ -13,6 +14,26 @@ static void StartListeners() | |||
listener.ReachabilityChanged += OnConnectivityChanged; | |||
} | |||
|
|||
static SignalStrength PlatformSignalStrength() | |||
{ | |||
var doubleSignalStrength = new NEHotspotNetwork().SignalStrength; |
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.
This allocation. I think it should be OK, but maybe wrap it in a using to dispose sooner?
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.
Yeah, I was worried about this as well. I was unable to decide whether to take the permanent memory hit and just lazy initialize a static NEHotspotNetwork which just sticks around forever once SignalStrength is used or allocate a new object on every call. This probably depends on the usage scenario as well, as if this were called in a rather tight loop or rather frequently would just add unnecessary memory pressure. If we decide to not cache the value we should definitely dispose the object ASAP to let i be destroyed on iOS.
✅ Validation status: passed
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
OPS Build status updates of commit 20a58f8: ❌ Validation status: errors
For more details, please refer to the build report. Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report. |
Description of Change
Implements #480
API Changes
Added:
public enum SignalStrength
{
Unknown,
None,
Weak,
Fair,
Strong
}
PR Checklist