Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement proxy preferences #726

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

T0astBread
Copy link

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
Copy link

cla-bot bot commented Nov 6, 2020

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
Copy link
Author

I accept the CLA ✔️

@T0astBread
Copy link
Author

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:

Screenshot of nethogs showing "de.grobox.liberari..." as one of its entries

and this is with proxy preferences set to use Tor:

Screenshot of nethogs not showing anything that looks like the Transportr app but with increased bandwidth usage from Tor

@grote grote requested a review from ialokim November 16, 2020 11:33
Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks a lot for your contribution! 😃

Looks like this would close #265

Co-authored-by: Torsten Grote <grote@users.noreply.github.com>
@T0astBread T0astBread force-pushed the proxy-settings branch 2 times, most recently from 068f0b1 to 66c83f6 Compare November 17, 2020 12:02
I hope this makes the clabot shut up
grote added a commit that referenced this pull request Nov 17, 2020
@cla-bot 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
Repository owner deleted a comment from cla-bot bot Nov 17, 2020
Repository owner deleted a comment from cla-bot bot Nov 17, 2020
Repository owner deleted a comment from cla-bot bot Nov 17, 2020
Repository owner deleted a comment from cla-bot bot Nov 17, 2020
Copy link
Collaborator

@ialokim ialokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

This is what the proxy settings currently look like:

proxy-settings-on
proxy-settings-off

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
Copy link
Collaborator

ialokim commented Nov 19, 2020

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
Copy link
Author

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
Copy link
Author

T0astBread commented Nov 24, 2020

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.

Copy link
Collaborator

@ialokim ialokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Author

T0astBread commented Jan 2, 2021

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.

Copy link
Collaborator

@ialokim ialokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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
Copy link
Owner

grote commented Feb 16, 2021

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
Copy link
Author

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
Copy link
Owner

grote commented Feb 16, 2021

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
Copy link
Collaborator

ialokim commented Feb 17, 2021

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
Copy link
Author

So, something like this?

Diagram describing user interface states: A settings screen with a link to a detailed "Proxy Settings" screen. The proxy settings screen checks, upon clicking a "save" check mark, the entered proxy settings and either returns the user to the previous screen or displays an error. The settings screen also indicates if the proxy settings work with a non-blocking connection status label.

@ialokim
Copy link
Collaborator

ialokim commented Feb 18, 2021

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
Copy link
Owner

grote commented Feb 19, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants