Implement proxy preferences #726

T0astBread · 2020-11-06T11:02:57Z

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).

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 · 2020-11-06T11:02:58Z

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 · 2020-11-06T11:04:24Z

I accept the CLA ✔️

T0astBread · 2020-11-06T11:27:12Z

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

app/src/main/java/de/grobox/transportr/settings/SettingsManager.kt

app/src/main/java/de/grobox/transportr/utils/TransportrUtils.kt

Co-authored-by: Torsten Grote <[email protected]>

I hope this makes the clabot shut up

Accepted here: #726 (comment)

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)?

app/src/main/java/de/grobox/transportr/settings/SettingsManager.kt

T0astBread · 2020-11-19T12:44:27Z

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 · 2020-11-19T13:06:29Z

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!

via the newly created ValidatedEditTextPreference

T0astBread · 2020-11-24T19:44:04Z

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 · 2020-11-24T19:48:08Z

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.

app/src/main/java/de/grobox/transportr/settings/SettingsFragment.kt

app/src/main/java/de/grobox/transportr/utils/TransportrUtils.kt

This also fixes a bug where stale proxy settings were sometimes used since PreferenceChangeListeners fire before the settingsManager of the SettingsFragment sees the new values in its settings instance.

T0astBread · 2021-01-02T17:05:32Z

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 · 2021-02-15T22:31:33Z

app/src/main/java/de/grobox/transportr/settings/SettingsFragment.kt

+                        TransportrUtils.updateGlobalHttpProxy(newProxy, manager)
+                    } catch (e: Exception) {
+                        Log.e(TAG, "Invalid proxy settings: " + e.message)
+                        AlertDialog.Builder(context!!)


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

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?

app/src/main/java/de/grobox/transportr/settings/SettingsFragment.kt

ialokim · 2021-02-15T22:36:40Z

app/src/main/java/de/grobox/transportr/settings/SettingsManager.kt

@@ -92,6 +100,23 @@ class SettingsManager @Inject constructor(private val context: Context) {
            }
        }

+    @Throws(UnknownHostException::class, IllegalStateException::class)
+    fun getProxy(proxyPrefOverrides: Map<String, Any>): Proxy {


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

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 · 2021-02-16T17:43:47Z

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 · 2021-02-16T17:45:20Z

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 · 2021-02-16T18:35:38Z

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 · 2021-02-16T18:38:09Z

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 · 2021-02-17T21:35:03Z

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 · 2021-02-18T02:51:45Z

So, something like this?

ialokim · 2021-02-18T22:13:16Z

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 · 2021-02-19T11:21:50Z

@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.

grote requested a review from ialokim November 16, 2020 11:33

grote requested changes Nov 16, 2020

View reviewed changes

app/src/main/java/de/grobox/transportr/settings/SettingsManager.kt Outdated Show resolved Hide resolved

app/src/main/java/de/grobox/transportr/utils/TransportrUtils.kt Show resolved Hide resolved

Use Log.e for parsing errors in proxy settings

66c83f6

Co-authored-by: Torsten Grote <[email protected]>

T0astBread force-pushed the proxy-settings branch 2 times, most recently from 068f0b1 to 66c83f6 Compare November 17, 2020 12:02

Add myself to contributors

ce83c52

I hope this makes the clabot shut up

grote added a commit that referenced this pull request Nov 17, 2020

Add @T0astBread to CLA

250e76e

Accepted here: #726 (comment)

Fix broken call to Log in SettingsManager

c73843e

cla-bot bot added the cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors. label Nov 17, 2020

Repository owner deleted a comment from cla-bot bot Nov 17, 2020

ialokim requested changes Nov 18, 2020

View reviewed changes

app/src/main/java/de/grobox/transportr/settings/SettingsManager.kt Outdated Show resolved Hide resolved

T0astBread added 2 commits November 24, 2020 20:37

Use a SwitchPreference for toggling network proxy usage

a1ea52d

Implement validation for proxy preferences

f945e3f

via the newly created ValidatedEditTextPreference

ialokim requested changes Dec 20, 2020

View reviewed changes

app/src/main/java/de/grobox/transportr/settings/SettingsFragment.kt Outdated Show resolved Hide resolved

app/src/main/java/de/grobox/transportr/utils/TransportrUtils.kt Outdated Show resolved Hide resolved

T0astBread added 2 commits January 2, 2021 17:52

Use Patterns for proxy host validation

84a3e7a

Test proxy connection on proxy preference changes

a93f491

This also fixes a bug where stale proxy settings were sometimes used since PreferenceChangeListeners fire before the settingsManager of the SettingsFragment sees the new values in its settings instance.

ialokim requested changes Feb 15, 2021

View reviewed changes

Explicitly allow "localhost" for proxy host preference

b889dcd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement proxy preferences #726

Implement proxy preferences #726

T0astBread commented Nov 6, 2020

cla-bot bot commented Nov 6, 2020

T0astBread commented Nov 6, 2020

T0astBread commented Nov 6, 2020

grote left a comment

ialokim left a comment

T0astBread commented Nov 19, 2020

ialokim commented Nov 19, 2020

T0astBread commented Nov 24, 2020

T0astBread commented Nov 24, 2020 •

edited

Loading

ialokim left a comment

T0astBread commented Jan 2, 2021 •

edited

Loading

ialokim left a comment

ialokim Feb 15, 2021

T0astBread Feb 16, 2021

ialokim Feb 15, 2021

T0astBread Feb 16, 2021

T0astBread commented Feb 16, 2021

grote commented Feb 16, 2021

T0astBread commented Feb 16, 2021

grote commented Feb 16, 2021

ialokim commented Feb 17, 2021 •

edited

Loading

T0astBread commented Feb 18, 2021

ialokim commented Feb 18, 2021

grote commented Feb 19, 2021

Implement proxy preferences #726

Are you sure you want to change the base?

Implement proxy preferences #726

Conversation

T0astBread commented Nov 6, 2020

cla-bot bot commented Nov 6, 2020

T0astBread commented Nov 6, 2020

T0astBread commented Nov 6, 2020

grote left a comment

Choose a reason for hiding this comment

ialokim left a comment

Choose a reason for hiding this comment

T0astBread commented Nov 19, 2020

ialokim commented Nov 19, 2020

T0astBread commented Nov 24, 2020

T0astBread commented Nov 24, 2020 • edited Loading

ialokim left a comment

Choose a reason for hiding this comment

T0astBread commented Jan 2, 2021 • edited Loading

ialokim left a comment

Choose a reason for hiding this comment

ialokim Feb 15, 2021

Choose a reason for hiding this comment

T0astBread Feb 16, 2021

Choose a reason for hiding this comment

ialokim Feb 15, 2021

Choose a reason for hiding this comment

T0astBread Feb 16, 2021

Choose a reason for hiding this comment

T0astBread commented Feb 16, 2021

grote commented Feb 16, 2021

T0astBread commented Feb 16, 2021

grote commented Feb 16, 2021

ialokim commented Feb 17, 2021 • edited Loading

T0astBread commented Feb 18, 2021

ialokim commented Feb 18, 2021

grote commented Feb 19, 2021

T0astBread commented Nov 24, 2020 •

edited

Loading

T0astBread commented Jan 2, 2021 •

edited

Loading

ialokim commented Feb 17, 2021 •

edited

Loading