Implement proxy preferences

[T0astBread]

This commit adds proxy preferences to the settings screen that are
then used in the ReverseGeocoder, MapBox and public-transport-enabler
(on NetworkProviders that support it, which should apply to every
provider).

One can verify that the app does not establish a direct internet
connection using a tool like nethogs (in Termux).

[cla-bot]

Thank you for your pull request and welcome to our community! We require contributors to sign our Contributor License Agreement, and we don't seem to have the user @T0astBread on file. In order for your code to get reviewed and merged, please explicitly state that you accept the agreement.

[T0astBread]

I accept the CLA ✔️

[T0astBread]

More on the nethogs thing: If you have a rooted phone you can run nethogs as root in Termux to see when an app establishes a network connection. This is how using a debug build without a proxy looks:

and this is with proxy preferences set to use Tor:

[grote]

Awesome! Thanks a lot for your contribution! 😃

Looks like this would close #265

[ialokim]

Thanks a lot for this PR! I think it would be nice to have some sort of user-feedback right away to see if the proxy settings they entered do work correctly. Not sure though where to hook this in: After a change to some of the fields or before leaving the settings screen?

Would you mind posting a screenshot of how the settings look like with these additions (in active and inactive proxy modes)?

[T0astBread]

This is what the proxy settings currently look like:

As for validation, I think it would make it more clear what's wrong if something happened right after a bad value is entered. Ideally the OK button would be disabled all together when the state of the text input is not acceptable but I'm not sure that's possible with an EditTextPreference. I'll look into how this works.

[ialokim]

This is what the proxy settings currently look like

Looks good to me, but I would just prefer a switch over the checkbox for enabling the proxy as documented here: https://source.android.com/devices/tech/settings/settings-guidelines#checkbox

As for validation, (...) I'll look into how this works.

Thanks for investigating!

[T0astBread]

Implemented some validation now. When the user enters an invalid value for either the host or port the "OK" button on the edit dialog is disabled immediately. The implementation is a bit more complicated than I'd like it to be since Android's EditTextPreference doesn't support that, so I kinda just wrote my own replacement for it.

[T0astBread]

It should also be noted that the validation doesn't match exactly the same cases as the proxy implementation can handle but, as far as I can tell, the supported cases are a superset of the validated cases and only weird edge cases (like abbreviated IPv4 addresses - "127.1" etc.) are not validated.

[ialokim]

Thanks for adding the validation part!

I've just tested your changes inserting an invalid hostname (in this case only the letter "g"). The validation doesn't complain about this (strange enough?), but also no other feedback is given that the proxy connection could not be established. I was able to make network requests as before over the normal internet connection without further notice.

I think the best thing would be to somehow test the proxy settings by performing a request after leaving the settings screen and report invalid values to the user straight away. At least no further network requests should be sent through the normal connection without informing the user.

[T0astBread]

Finally another update from me! Proxy connections are now tested after the user changes a proxy preference. If the test fails, an AlertDialog with the error message is shown. Preferences are still accepted on a failed test since it would otherwise be impossible to change e.g. the host and port at the same time.

[ialokim]

Sorry for the delay. I've finally had another look at your changes. Please see my comments in-code.

I still think it would be nice to finally test the proxy settings when leaving the settings screen (onBackButtonPressed or something similar) and in case the connection could not be established show another Toast message and deactivate the proxy again.

[ialokim]

I would prefer a less disturbing Toast message at this point.

[T0astBread]

Do you think it would be a good idea to handle some common errors using a Toast with a simplified message and use an AlertDialog with the Exception's message for anything unexpected?

[ialokim]

All uses of this function seem to set at most one new setting. Why not simply using a Pair<String, Any> here?

[T0astBread]

You're right but I think the code is actually more readable with the map compared to a single Pair<String, Any>?, if we want to handle mismatched types the same way.

Compare

val isEnabled = proxyPrefOverrides[PROXY_ENABLE] as Boolean? ?: settings.getBoolean(PROXY_ENABLE, false)

to

val isEnabled =
    if (proxyPrefOverride?.first == PROXY_ENABLE && proxyPrefOverride.second is Boolean)
        proxyPrefOverride.second as Boolean
    else
        settings.getBoolean(PROXY_ENABLE, false)

or (avoiding the explicit cast)

val overrideKey = proxyPrefOverride?.first
val overrideVal = proxyPrefOverride?.second
val isEnabled = if (overrideKey == PROXY_ENABLE && overrideVal is Boolean)
    overrideVal else settings.getBoolean(PROXY_ENABLE, false)

[T0astBread]

I've avoided automatically deactivating the proxy since I figured someone might not notice the Toast and accidentally use the app without a proxy. If the app just continues using invalid proxy settings (which blocks all connections), there's no way a user won't notice something's wrong.

[grote]

I don't have time at the moment to do a review, so just a comment: When setting a proxy, I'd show a progress bar until we know the proxy works and then only set the setting for real when it does. Otherwise don't set the settings and show an error dialog.

[T0astBread]

Where should the progress bar be placed? At the top or bottom of the settings activity, perhaps together with a Snackbar displaying something like "Checking proxy settings..."? Or a progress dialog (those are deprecated)? And what should happen when the user exits the settings activity while the proxy connection is being checked?

I could add another set of proxy preferences that act as "untested" proxy preferences, which are what the user actually sees in the UI. Then, when the user exits the settings activity and the untested settings differ from the currently used ones, the untested settings are checked with a non-blocking progress bar. If the user presses back to exit the activity a Toast shows up saying something like "Press back again to cancel the proxy check". Something similar is probably also possible for the back arrow in the app bar.

But is that really necessary? I know, blocking the UI for a network request is really bad UX but who, other than power users, would mess with proxy settings anyways?

[grote]

The network request would have to be done off the UI thread of course, but the UI could still artificially be blocked. Just show a Dialog fragment with a progress bar and a text saying "testing proxy settings". the same fragment could even show the error message and attach to the same ViewModel as the settings.

[ialokim]

I've just have done some comparison on how other apps handle proxy settings:

• F-Droid and Aurora Store simply crash after changing to invalid proxy settings, I had to clear the application data to get them to work again - definitely not the way to go.
• DAVx5 at least doesn't crash, but simply reports a general network error as it would do whenever the Cal/CardDav server would not be reachable.
• Telegram FOSS is the only app I've tested that actually gives the user some feedback directly to the proxy settings. See the following screen record:

Telegram-Proxy.mp4

I think it would make sense to move the proxy settings to a new "sub-preference" screen similar to how Telegram does it. Of course I would argue one proxy setting at a time should be enough. But then we could really check the proxy settings before returning to the main preference screen and prevent "false" or non-working settings. Also, in the main preference screen, there could be a description field stating whether the proxy settings are working and enabled or not.

Also see https://developer.android.com/guide/topics/ui/settings/organize-your-settings#split_your_hierarchy_into_multiple_screens

[T0astBread]

So, something like this?

[ialokim]

Yes, that looks really good to me! Just two minor things:

• Perhaps we could combine the switch and the area to go to the proxy settings into one single row, as e.g. in Android system's settings for Wifi. See this for an image, but not sure if the answer helps much. I've also found this but I could imagine that something similar might exist natively by now.
• The check should also be triggered when going back from the proxy settings to the main settings using the back arrow. Not sure if a dedicated check mark in the top right would be necessary then.

[grote]

@T0astBread thanks for the mockup, this looks great!

I personally don't think that we need these colored status information on the main settings screen for proxy settings. Just say if a proxy is set or if it isn't.

If we allow the user to leave the proxy screen while still checking the settings, they either won't see the result (success or fail) or we don't set the proxy settings at all. The first option is probably better, but then it might be overkill to add checking status info to the main screen. Just update the proxy livedata when we have the result.