Skip to content

Conversation

@link2xt
Copy link
Collaborator

@link2xt link2xt commented Nov 14, 2025

This is part of #7357 needed to get rid of is_chatmail distinction between profiles. Previously the setting was default to 1 for non-chatmail and 0 for chatmail.
See also previous PR #7440

@link2xt link2xt force-pushed the link2xt/bcc-self-default-0 branch from 80a3dcf to 81653df Compare November 14, 2025 20:26
src/config.rs Outdated
///
/// Should be enabled for multidevice setups.
/// Default is 0 for chatmail accounts, 1 otherwise.
/// The default is only used for old accounts.
Copy link
Collaborator

@iequidoo iequidoo Nov 14, 2025

Choose a reason for hiding this comment

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

It's better to change the default to 0 then and add a db migration for old accounts which sets 1 for non-chatmail. We always did it this way before. This way if it's set to 1, we know it's set by us, not by the user (EDIT: probably):

  • Either by the migration
  • Or by the multi-device detection logic.

And if it's set to 0 explicitly (not to None), that's definitely done by the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Knowing whether the user has ever changing the setting manually looks like future-proofing for a never happening future. We already don't know if it was set to 1 by the user because we set it automatically. And we don't need to know if it was set to 0 by the user, we still want to set it to 1 when setting up a second device regardless of whether the user has ever turned it off.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And we don't need to know if it was set to 0 by the user, we still want to set it to 1 when setting up a second device regardless of whether the user has ever turned it off.

True, but changing a user-modified setting from version to version silently isn't what a software normally does. Usually it's suggested to merge the config or at least to review the new settings.

@link2xt link2xt force-pushed the link2xt/bcc-self-default-0 branch 2 times, most recently from 0dfe74a to db1b82d Compare November 15, 2025 14:25
@link2xt link2xt force-pushed the link2xt/bcc-self-default-0 branch from cf207f4 to 35bd1d3 Compare November 16, 2025 07:58
@link2xt link2xt force-pushed the link2xt/bcc-self-default-0 branch from a67ae78 to cd9e464 Compare November 16, 2025 08:23
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 16, 2025

As for changing the default and migration for old accounts vs setting the new default in configure(), migrations have many problems:

  1. Migrations may affect existing users. If the migration is incorrect, it will break setups of many users.
    Changing the default set during configuration cannot affect existing users because they don't run configuration.
  2. Migrations are not tested. They have to be tested manually in all configurations. Setting the new default during configuration is tested immediately by CI and the only case that needs to be tested is fresh setup.
  3. Users sometimes downgrade during "setup second device", e.g. if they have new desktop and setup somewhat outdated F-Droid build as second device or copy from new Android/iOS to outdated desktop setup.
    When we change how we treat missing settings by setting default, it means that the setting may change the value during the downgrade.
    It seems to be not the case for this change because we change the default to "0" and for all cases where we have previously treated it as "1" we store it manually in the database, but this is only checked manually.
  4. Migrations prevent testing with existing setups. We cannot just test PRs with migrations with our accounts without making a backup. Once merged to main, they cannot be reverted without coordination with everyone who may have ran it.

I think we need to write down something regarding avoiding SQL migrations when possible in STYLE.md.

I have added a migration to this PR now to get rid of complicated logic in the default that depends on IsChatmail which we need to remove eventually anyway because of multi-transport.

But I still need to test the migration manually for chatmail and non-chatmail before merging, I think so far it was not executed at all by anyone because CI is not testing it.

@link2xt
Copy link
Collaborator Author

link2xt commented Nov 16, 2025

I did some manual tests.

  1. Setup a non-chatmail account with mailo.com before migration.
> set addr xxx@mail.com
> set mail_pw ...
> configure
> get bcc_self
bcc_self=Ok(Some("1"))
  1. Setup a chatmail account before migration.
setqr dcaccount:ci-chatmail.testrun.org

At this point REPL said "Config set from QR code, you can now call 'configure'", but the account is already configured at this point. (opened #7450 )

> get bcc_self
bcc_self=Ok(Some("0"))
  1. Switched to this branch with migration.
> get bcc_self
bcc_self=Ok(Some("1"))
  1. Running new branch with mailo.com database.
> get bcc_self
bcc_self=Ok(Some("1"))

Version is 139 in the "info" output.

  1. Running new branch with chatmail database.
> get bcc_self
bcc_self=Ok(Some("0"))

Version is 139.

  1. Removed everything, switched to old version again.

  2. Setup mailo.com in the first database, set bcc_self to 0 manually.

Configured mailo.com again, checked the database version is 138, ran set bcc_self 0.

  1. Setup a new chatmail account in the second database, but set bcc_self to 1 manually.

  2. Updated to this PR with db version 139.

  3. Opened mailo database

> get bcc_self
bcc_self=Ok(Some("0"))
  1. Opened chatmail database
> get bcc_self
bcc_self=Ok(Some("1"))

@link2xt link2xt merged commit 22ebd64 into main Nov 16, 2025
51 of 54 checks passed
@link2xt link2xt deleted the link2xt/bcc-self-default-0 branch November 16, 2025 10:00
@hpk42
Copy link
Contributor

hpk42 commented Nov 18, 2025 via email

@link2xt
Copy link
Collaborator Author

link2xt commented Nov 18, 2025

Asking users to "merge the config" and "review new settings" might be fine for linux-distro level things

It's not fine even for Linux distro, I don't like that maintainers ship default configs in the same location as production configs instead of some examples directory and I have to merge them manually each time I update. This might have been fine when updates were received on CDs every few years or if you never update to the next major release.

@iequidoo
Copy link
Collaborator

iequidoo commented Nov 18, 2025

With "review new settings" i meant adding a device message like "Send copy to self" was auto-set to 0 on update to version X. You may want to review it because you set it manually. Most users won't see it because they don't touch it or even use chatmail, so it may be untranslated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants