Fix deprecated networking getters

guardianproject / orbot

The Github home of Orbot: Tor on Android (Also available on gitlab!)

https://gitlab.com/guardianproject/orbot

Other

2.27k stars 338 forks source link

Fix deprecated networking getters #1141

Closed meenbeese closed 4 months ago

meenbeese commented 6 months ago

Resolves:

'getter for activeNetworkInfo: NetworkInfo?' is deprecated. Deprecated in Java
'getter for isConnected: Boolean' is deprecated. Deprecated in Java

bitmold commented 5 months ago

Why did you add in capabilities.hasTransport(NetworkCapabilities.TRANSPORT_BLUETOOTH) -> true?

meenbeese commented 5 months ago

I got the code from SO. Should be fixed now.

meenbeese commented 5 months ago

Didn't realize the same code is present in ConnectFragment. Also fixed that instance.

bitmold commented 5 months ago

This was a good example of how while it's nice to fix deprecated android libraries and make these kinds of revisions there is always a tradeoff with changing code. With Orbot I feel the need to prioritize fixing documented bugs than necessarily addressing all of these issues, because when doing so like with the bluetooth thing we can always accidentally break existing functionality.

But thanks, there is still value in this of course. Have you tested these changes ?

meenbeese commented 5 months ago

This was a good example of how while it's nice to fix deprecated android libraries and make these kinds of revisions there is always a tradeoff with changing code. With Orbot I feel the need to prioritize fixing documented bugs than necessarily addressing all of these issues,

I try to make fixes in all areas where I have a decent amount of knowledge and understanding, which means that most networking stuff is beyond my reach currently. I also have a few user-facing changes in the backlog that I will push once I polish them. Keep in mind that simplifying code like this can also reveal where bugs are present by getting rid of needlessly complex logic.

because when doing so like with the bluetooth thing we can always accidentally break existing functionality.

That's why we do code reviews :)

But thanks, there is still value in this of course. Have you tested these changes ?

Yes, I have tested it on the emulator and it works fine.

bitmold commented 5 months ago

Can you please implement this so as to define one function statically that takes a Context object and performs this check instead of implementing it in two fragments. That way it exists once it a centralized place and when it needs to be changed or used elsewhere it's a lot more straightforward. Thanks

meenbeese commented 5 months ago

Done. Please take a look.

bitmold commented 4 months ago

Great, thanks