befora / Kuboo

Kotlin + Ubooquity = Kuboo
Apache License 2.0
83 stars 41 forks source link

Wifi only is not honored #66

Closed gotson closed 5 years ago

gotson commented 5 years ago

On version 1.2.2 I have wifi only set, but manual download and tracking downloads still happen over mobile connectivity.

befora commented 5 years ago

I am able to recreate this issue.

befora commented 5 years ago

commit https://github.com/sethchhim/Kuboo/commit/e85501f7605d8b82d7703bb4d3f3b73c1d2c1643

Will be live in 1.2.3, uploaded now and awaiting Google approval.

I'm out until next week, so I won't be able to test it. Hope it solves your issue.

gotson commented 5 years ago

Thanks for the quick answer, I tried it but I still have the issue.

No big deal anyway, it's not a blocker on my side!

befora commented 5 years ago

My tests show correct behavior for 1.2.3

Android 9.0 Api 28

Wifi Only[Enabled] with Wifi Data[Enabled] with Mobile Data[Enabled] -> Pass Wifi Only[Enabled] with Wifi Data[Enabled] with Mobile Data[Disabled] -> Pass Wifi Only[Enabled] with Wifi Data[Disabled] with Mobile Data[Enabled] -> Pass

Wifi Only[Disabled] with Wifi Data[Enabled] with Mobile Data[Enabled] -> Pass Wifi Only[Disabled] with Wifi Data[Enabled] with Mobile Data[Disabled] -> Pass Wifi Only[Disabled] with Wifi Data[Disabled] with Mobile Data[Enabled] -> Pass

gotson commented 5 years ago

it's good that you have unit tests 👍, but actual behaviour differs on my OnePlus 6T with Android 9.

I switch my Wifi off, download a file -> it downloads over mobile data. I switch Wifi on, start download a file, pause, switch Wifi off, resume download -> it downloads over mobile data. I switch Wifi off, enable tracking for a downloaded file -> it downloads over mobile data.

befora commented 5 years ago

I'm testing on software emulator and it works perfectly. This is a pain point in Android development. So many devices and manufacturers with unpredictable behavior even when using Google's own connectivity manager API. I will revisit at a later time.

befora commented 5 years ago

I'd like to cover all tracks, can you post your version number.

befora commented 5 years ago

commit https://github.com/sethchhim/Kuboo/commit/717e95cb69e685f7be2c9fb7c3c82595e82fcb30

We will try NetworkCapabilities. This is only available in Android Marshmallow and above. Testable on 1.2.5 on the beta channel. Uploaded now and awaiting Google approval.

Also added an advanced option to allow VPN through the wifi only check.

befora commented 5 years ago

commit https://github.com/sethchhim/Kuboo/commit/5a529ec969a9a7ed210621dbb211c434cc264fe5

Refactor from Wifi Only into Disable Cellular, please feel free to reopen this does not fix your issue.

gotson commented 5 years ago

commit 5a529ec

Refactor from Wifi Only into Disable Cellular, please feel free to reopen this does not fix your issue.

When using this, i cannot even browse the server anymore. It seems quite drastic and not exactly the expected behaviour. Plus the error message says server not available, which is not exactly true.

befora commented 5 years ago

Logic is sound, I tested again and it works on my end. I will hunt down someone's OnePlus phone to test on later. Here is the segment for review (SystemUtil.kt:92):

    private fun isActiveNetworkCellular(): Boolean {
        val connectivityManager = getConnectivityManager()
        return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
            val networkCapabilities = connectivityManager.getNetworkCapabilities(connectivityManager.activeNetwork)
            networkCapabilities != null && networkCapabilities.hasTransport(TRANSPORT_CELLULAR)
        } else {
            val activeNetworkInfo = connectivityManager.activeNetworkInfo
            activeNetworkInfo != null && activeNetworkInfo.type == ConnectivityManager.TYPE_MOBILE
        }
    }
befora commented 5 years ago

If anyone else out there could comment on their experience with the Disable Cellular setting, please do because I have no leads to this issue.

gotson commented 5 years ago

What I'm actually saying is that the way it is implemented is binary. You either have access to the server via mobile, or you don't.

What would actually make sense to me would be to disable the background data usage, but not the interactive one. That would mean I should be able to browse the server, and read a comic, but not have tracking automatically downloading files.

It would be similar to how Google Photos is working, you can disable sync over mobile, so that your photos are only uploaded over wifi, but you can still view any picture from your library even if it's not on your device.

befora commented 5 years ago

Yes, I want to block all mobile data with this setting.