Add ID command for IMAP

[wh201906]

Fixes #5089
This PR is based on the implementation of useCompression and PR #5578.
I have tested this PR on Netease mail, which requires the ID extension. It works as expected.

[wh201906]

While this PR is functional now, I have some uncertain points about it.

1. The loadDefaults() in AccountPreferenceSerializer.kt contains the default value of isIgnoreChatMessages, but I can't find the default value of useCompression in this function. Should I add sendClientID in it?
2. The getBoolean() and putBoolean() in AccountSettingsDataStore.kt contain the ignore_chat_messages (get, set), but I can't find useCompression or something related in these functions. Should I add sendClientID/send_client_id in them?
3. account_settings.xml contains the preference of ignore_chat_messages, but I can't find preference of useCompression or something related in this file. Should I add sendClientID/send_client_id in it?
4. In this PR, the default value of sendClientID is false. Should I change it?
5. I didn't write a unit test of this setting. For now I have no idea how this option is tested.
6. How do you think of the name sendClientID? Should I change it?
7. The name of "K-9" is hard coded in the ID command. should I change it or read it from elsewhere?

[cketti]

1. The loadDefaults() in AccountPreferenceSerializer.kt contains the default value of isIgnoreChatMessages, but I can't find the default value of useCompression in this function. Should I add sendClientID in it?

Properties in Account.kt need to be initialized anyway. Adding the default value to AccountPreferenceSerializer.loadDefaults() is mostly unnecessary duplication. There's no need for this PR to add code to this method.

2. The getBoolean() and putBoolean() in AccountSettingsDataStore.kt contain the ignore_chat_messages (get, set), but I can't find useCompression or something related in these functions. Should I add sendClientID/send_client_id in them?

AccountSettingsDataStore is how AccountSettingsFragment persists preferences. However, the "incoming server settings" screen is a separate activity that is not using this mechanism. So there's no need to touch this file.

3. account_settings.xml contains the preference of ignore_chat_messages, but I can't find preference of useCompression or something related in this file. Should I add sendClientID/send_client_id in it?

Same as 2). Different mechanism.

4. In this PR, the default value of sendClientID is false. Should I change it?

A handful of providers seem to require the ID command. So it should probably be enabled by default.

5. I didn't write a unit test of this setting. For now I have no idea how this option is tested.

Tests should go into RealImapConnectionTest.
Test cases we should probably have:

• ID capability not present → make sure ID command is not sent
• ID capability present and setting enabled → make sure ID command is sent
• ID capability present and setting disabled → make sure ID command is not sent

6. How do you think of the name sendClientID? Should I change it?

Without further context I'd assume this is a method name, because it starts with a verb. I suggest changing it to isSendClientIdEnabled (also note that the D in ID is not capitalized for easier readability).

7. The name of "K-9" is hard coded in the ID command. should I change it or read it from elsewhere?

The name of the app ("K-9 Mail") should be supplied via a BuildConfig field. See e.g. https://github.com/thundernest/k-9/blob/975049ef71e06d4f598b2838f58cbc3fe63a1f77/app/k9mail/build.gradle.kts#L89-L93

Instead of adding a Bool and a String to ImapSettings, add clientId: String?. That way we can send the ID command if clientId is not null, and skip sending the ID command otherwise.

[wh201906]

A handful of providers seem to require the ID command. So it should probably be enabled by default.

The name of the app ("K-9 Mail") should be supplied via a BuildConfig field. See e.g.

Instead of adding a Bool and a String to ImapSettings, add clientId: String?. That way we can send the ID command if clientId is not null, and skip sending the ID command otherwise.

Sorry. I'm not clear about the code for them. Do you mean add a single String setting item in the ImapSettings? If so, how do the users configure it? IDK if it should be configured by a checkbox or a textfield. Plus, how does this setting item work with the BuildConfig field?

---

I guess we can add a checkbox in the AccountSetupIncoming.java. If it's checked, write the text from corresponding BuildConfig field to the setting item clientId. If not, set the clientId to empty. When establishing the connection,

send the ID command if clientId is not null, and skip sending the ID command otherwise.

IDK if the logic above is appropriate.

[cketti]

Do you mean add a single String setting item in the ImapSettings?

Yes.

If so, how do the users configure it?

They don't. The app will set the value to its app name (read from a BuildConfig field) if the setting to send the client ID is enabled and to null otherwise.

[wh201906]

So I guess the logic I said above is fine? If so I'll write code based on this.

I guess we can add a checkbox in the AccountSetupIncoming.java. If it's checked, write the text from corresponding BuildConfig field to the setting item clientId. If not, set the clientId to empty. When establishing the connection,

send the ID command if clientId is not null, and skip sending the ID command otherwise.

[wh201906]

By the way, I wonder why not adding a Boolean setting item, then read the BuildConfig.clientID when establishing the connection.
If the clientID is unchangeable for users I think there is no need to store them again in the ImapSettings. Is that for performance?

[wh201906]

I will squash my commits when things are ready.

[cketti]

The IMAP code is in a library project that knows nothing about the app using the library. It's also a JVM-only project, that doesn't have access to BuildConfig because that's an Android build system thing.

[wh201906]

The IMAP code is in a library project that knows nothing about the app using the library. It's also a JVM-only project, that doesn't have access to BuildConfig because that's an Android build system thing.

I see.

[cketti]

Apologies for not being clear enough. Account should still only keep track of a boolean value, isSendClientIdEnabled.

The code to translate Account.isSendClientIdEnabled and the app name into a configuration for the IMAP library should go into ImapBackendFactory.

[wh201906]

Apologies for not being clear enough. Account should still only keep track of a boolean value, isSendClientIdEnabled.

The code to translate Account.isSendClientIdEnabled and the app name into a configuration for the IMAP library should go into ImapBackendFactory.

OK. I will refactor my code tomorrow because it's late night(UTC+8).
I think most of the part has been implemented in commit 209a813. You can check it and give me more suggestions if you want.
(Nevermind. I just noticed you might checked commit 76e7637 already.)
By the way, commit 209a813 passed the CI on my fork. I have tested it and it works as 2096b97

[wh201906]

If the code is good I'll implement the unit test.

[wh201906]

Thank you for spending time reviewing this. I have changed the code as you suggested and tested in on my device. It works as 2096b97

[wh201906]

Test cases we should probably have:

* ID capability not present → make sure ID command is not sent
* ID capability present and setting enabled → make sure ID command is sent
* ID capability present and setting disabled → make sure ID command is not sent

Sorry, I don't know what's the server response look like if the ID capability is present. Could you please implement the tests?

[cketti]

Thanks 👍

[wh201906]

Thank you so much for guiding me and reviewing this!