Add support for server settings autodiscovery #3879

wiktor-k · 2019-01-16T13:05:46Z

This change makes account setup query mail provider's HTTPS server and fills in appropriate settings for store and transport settings.

If there is no such file or the file is malformed the account setup proceeds as usual with the manual setup.

If the user doesn't want to automatically discover server settings they can initiate manual setup by pressing "Manual Setup" button on the first screen.

Currently implemented autodiscovery scheme is Thunderbird Autoconfig but this can be extended to additional methods (e.g. DNS or autodiscover).

Resolves #865.

See: https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat

Please ensure that your pull request meets the following requirements - thanks !

Follows our existing codestyle.
Does not break any unit tests. ✔️
Contains a reference to the issue that it fixes. ✔️
For cosmetic changes add one or multiple images, if possible. There is no UI, it either works or not very quickly

app/ui/src/main/java/com/fsck/k9/activity/setup/AccountSetupBasics.java

app/ui/src/main/java/com/fsck/k9/activity/setup/AccountDiscovery.kt

app/ui/src/test/java/com/fsck/k9/activity/setup/AccountDiscoveryTest.kt

wiktor-k · 2019-02-06T18:16:26Z

Great, excellent feedback @cketti, thank you! I'll address your points as soon as possible.

wiktor-k · 2019-02-21T14:06:18Z

Updated the HTTP code to use okhttp @cketti (funnily it seems on Android 8 even native HttpUrlConnection is internally using okhttp).

Okhttp required new proguard rules and changing java version level to 1.8 (as okhttp is using static interface methods, a similar issue is reported here: JakeWharton/butterknife#1416) but otherwise the code is almost identical.

cketti

I've thought about this some more. The current approach is probably too simple and doesn't lead to a great user experience when things go wrong (e.g. when we retrieve server settings that don't actually work). We need to move away from fetching server settings and actually testing these settings in different activities. I'll try to write up my thoughts on a proper design for automatically detecting server settings next week.

But I guess we can merge this as interim solution once the comments are addressed.

app/ui/build.gradle

app/ui/src/main/java/com/fsck/k9/activity/setup/discovery/AccountDiscovery.kt

cketti · 2019-02-26T15:50:24Z

app/ui/src/main/java/com/fsck/k9/activity/setup/discovery/AccountDiscovery.kt

+class AccountDiscovery(private val discoverers: Array<ConnectionSettingsDiscoverer>) {
+
+    fun discover(email: String, callback: (settings: ConnectionSettings?) -> Unit) {
+        launch(UI) {


I'd like us to split code like this into two parts. Most of the time the "business logic" part doesn't have to be concerned about the main thread at all. It should assume it is running in a background thread. To make the feature available to UI code, create a class that starts the work in a background thread and delivers the result in the main thread. Ideally, the business code also supports cancellation. It's then the job of the "UI wrapper" class to cancel the work at the appropriate times (user closes the activity etc). Often such code could live in an androidx.lifecycle.ViewModel class.

Also, lets avoid passing callbacks from Activities to long-running code in a background thread.

Yep, I agree the code looked bad.

I did a quick look on classes using androidx.lifecycle.ViewModel but it seems to be aimed at things that can change more than once (like accounts).

I modified the discover method to just do the background work, without any UI dependencies. And in UI (AccountSetupBasic) I wrapped the call in an AsyncTask. AsyncTask supports cancellation in principle but I didn't implement it (it should check and break the for loop).

Do you think AsyncTask is good enough for the job or should I rework it to a ViewModel?

app/ui/src/main/java/com/fsck/k9/activity/setup/discovery/ConnectionSettingsDiscoverer.kt

app/ui/src/main/java/com/fsck/k9/activity/setup/discovery/ThunderbirdAutoconfigFetcher.kt

wiktor-k · 2019-02-27T09:49:23Z

We need to move away from fetching server settings and actually testing these settings in different activities.

I don't know if I understood you correctly but the settings that are fetched are tested just like manual settings (and settings from providers.xml). If they're incorrect the user is provided with the standard "correct"/"continue anyway" dialog. There is a "manual setup" button that will well enter manual setup without autodetecting so broken settings will not lock out anyone.

cketti · 2019-02-28T01:15:25Z

I don't know if I understood you correctly but the settings that are fetched are tested just like manual settings (and settings from providers.xml). If they're incorrect the user is provided with the standard "correct"/"continue anyway" dialog. There is a "manual setup" button that will well enter manual setup without autodetecting so broken settings will not lock out anyone.

That's true. But especially once we add more methods to retrieve server settings chances increase that one method returns outdated or plain wrong results. Right now the loop stops as soon as any server settings are retrieved. Checking the server settings is a second step. If that fails there's no going back to try additional server settings retrieved using different methods. It's not really an issue now. But it's not a future proof design either.

wiktor-k · 2019-02-28T09:03:37Z

But it's not a future proof design either.

Agreed. Making it more flexible, like you described checking autodiscover settings in succession would be more complex from the UX perspective currently. The design we have now (one settings) works well as this is just an extension of how providers.xml worked. I am afraid doing it the way you described would also need some tweaks to the UI of account setup.

That being said I hope having at least one autodiscovery would solve majority of "add me to providers.xml" PRs :)

This change makes account setup query mail provider's HTTPS server and fills in appropriate settings for store and transport settings. If there is no such file or the file is malformed the account setup proceeds as usual with the manual setup. If the user doesn't want to automatically discover server settings they can initiate manual setup by pressing "Manual Setup" button on the first screen. Currently implemented autodiscovery scheme is Thunderbird Autoconfig but this can be extended to additional methods (e.g. DNS or autodiscover). Resolves thunderbird#865. See: https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat

wiktor-k · 2019-03-01T12:49:25Z

Okay, fixed all mentioned issues (though I'm not sure what you think about AsyncTask)...

Thanks for taking the time to review this @cketti, I really appreciate it.

cketti · 2019-03-09T00:51:42Z

An AsyncTask is not going to work. It'll hold on to the wrong Activity instance e.g. in the case of an orientation change.

This PR is already quite big. We should break this down and do it step by step. I'll create a series of smaller PRs to get us to what I have in mind. I'll pick up your Thunderbird Autoconfig changes after some initial "infrastructure" changes.

cketti · 2019-03-22T00:08:52Z

Superseded by #3980

wiktor-k force-pushed the autodiscovery branch from 5c7acf0 to 6c0425d Compare January 17, 2019 14:34

wiktor-k mentioned this pull request Jan 17, 2019

adding provider to app/core/src/main/res/xml/providers.xml #3871

Closed

wiktor-k changed the title ~~WIP: Add support for server settings autodiscovery~~ Add support for server settings autodiscovery Jan 28, 2019

cketti reviewed Feb 6, 2019

View reviewed changes

wiktor-k force-pushed the autodiscovery branch from 6c0425d to c8f01ca Compare February 10, 2019 20:30

wiktor-k force-pushed the autodiscovery branch from c8f01ca to 10e0e12 Compare February 21, 2019 13:55

This comment has been minimized.

Sign in to view

cketti reviewed Feb 27, 2019

View reviewed changes

wiktor-k force-pushed the autodiscovery branch from 10e0e12 to 323175d Compare March 1, 2019 11:59

wiktor-k added 2 commits March 1, 2019 13:23

Use okhttp for fetching files over HTTPS

ed752ce

wiktor-k force-pushed the autodiscovery branch from 323175d to ed752ce Compare March 1, 2019 12:28

cketti added the don't merge label Mar 9, 2019

cketti closed this Mar 22, 2019

This was referenced May 3, 2020

Add detection of configuration based on SRV records (RFC6186) #4719

Closed

Automatic account setup / server settings autodiscovery #4721

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for server settings autodiscovery #3879

Add support for server settings autodiscovery #3879

wiktor-k commented Jan 16, 2019 •

edited

wiktor-k commented Feb 6, 2019

wiktor-k commented Feb 21, 2019

This comment has been minimized.

This comment has been minimized.

cketti left a comment

cketti Feb 26, 2019

wiktor-k Mar 1, 2019

wiktor-k commented Feb 27, 2019

cketti commented Feb 28, 2019

wiktor-k commented Feb 28, 2019

wiktor-k commented Mar 1, 2019

cketti commented Mar 9, 2019

cketti commented Mar 22, 2019

Add support for server settings autodiscovery #3879

Add support for server settings autodiscovery #3879

Conversation

wiktor-k commented Jan 16, 2019 • edited

wiktor-k commented Feb 6, 2019

wiktor-k commented Feb 21, 2019

This comment has been minimized.

This comment has been minimized.

cketti left a comment

Choose a reason for hiding this comment

cketti Feb 26, 2019

Choose a reason for hiding this comment

wiktor-k Mar 1, 2019

Choose a reason for hiding this comment

wiktor-k commented Feb 27, 2019

cketti commented Feb 28, 2019

wiktor-k commented Feb 28, 2019

wiktor-k commented Mar 1, 2019

cketti commented Mar 9, 2019

cketti commented Mar 22, 2019

wiktor-k commented Jan 16, 2019 •

edited