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

hide IMAP folder options on chatmail #3050

Merged
merged 4 commits into from
May 15, 2024
Merged

hide IMAP folder options on chatmail #3050

merged 4 commits into from
May 15, 2024

Conversation

r10s
Copy link
Member

@r10s r10s commented May 14, 2024

this PR makes use of the is_chatmail config introduced by deltachat/deltachat-core-rust#5567 and hides the IMAP folder options.

for whatever reason, is_chatmail is currently always 0 (also in "View Log" which is generated by core directly), might be things are not rolled out yet. EDIT: works now, see below

this is what it will look like:

Screenshot 2024-05-14 at 23 19 00   Screenshot 2024-05-15 at 00 54 22
left: chatmail=0, right: chatmail=1

targets deltachat/deltachat-core-rust#5380

when this PR gets merged, we need to file issues/PR for the same change on desktop/ios

@r10s r10s requested a review from link2xt May 14, 2024 21:24
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@link2xt
Copy link
Contributor

link2xt commented May 14, 2024

for whatever reason, is_chatmail is currently always 0 (also in "View Log" which is generated by core directly), might be things are not rolled out yet.

XCHATMAIL capability was not deployed, it is fixed now.

@r10s r10s requested a review from adbenitez May 14, 2024 22:12
@r10s
Copy link
Member Author

r10s commented May 14, 2024

great! works perfectly fine

@adbenitez
Copy link
Member

probably "prefer encryption" toggle should be locked/disabled in ON mode, to avoid people accidentally disabling encryption in chatmail servers

more importantly, the "import contacts from address book" should also be hidden


if (dcContext.isChatmail()) {
sentboxWatchCheckbox.setVisible(false);
bccSelfCheckbox.setVisible(false);
Copy link
Member

Choose a reason for hiding this comment

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

if someone that is already using chatmail disabled this toggle, now they will not be able to enable it anymore, would be nice to do some migration checking for this property and maybe also the other ones, and set/force the correct default values

Copy link
Member Author

@r10s r10s May 14, 2024

Choose a reason for hiding this comment

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

imu, whatever is chosen for these four options, it does not make a difference for chatmail. but not sure - otherwise, i would prefer some migration in the core, or that the settings are ignored, at least when they're harmful, i think, we do want to have less footguns and more defaults for chatmail :) cc @link2xt

Copy link
Member

Choose a reason for hiding this comment

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

bcc self makes a difference without it multidevice sync is broken, or do you mean that internally core already ignores their value if chatmail flag is set?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is fine to hide the settings. Core sets mvbox_move on configuration, we can also change other settings on configuration. If users encounter problems, they can report bug and look into context info to see the settings, if they happen to be broken then reconfiguring will help.

Copy link
Contributor

Choose a reason for hiding this comment

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

deltachat/deltachat-core-rust#5568

I don't want to reset settings in a migration or anything similar because immediately after migration we don't know if it is a chatmail until we connect to IMAP. And resetting every time when we connect to IMAP will break tests that test these settings by manually setting them on nine.testrun.org accounts.

@r10s
Copy link
Member Author

r10s commented May 14, 2024

"prefer e2ee" and "read system address book" are hidden now as well, thanks for the pointer!

if the latter was set before, it is now ignored on chatmail - the other options are reset by deltachat/deltachat-core-rust#5568 - so, i think, this is good enough - i do not expect many ppl playing around with these options anyways

Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s r10s requested a review from adbenitez May 14, 2024 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants