Migrate K9.isHideTimeZone to PreferenceDataStore

[shamim-emon]

• Fixes Migrate K9.isHideTimeZone to PreferenceDataStore #9427

Implementation Highlights:

• MigratedisHideTimeZone from K9 to PrivacySettings.
• Exposed it via PrivacySettingsManager in GeneralSettingManager.
• Added implementation of PrivacySettingsManager , DefaultPrivacySettingsManager to set/get privacy settings.
• Included PrivacySettingsPreferenceManager's implementation DefaultPrivacySettingsPreferenceManager.
• Replaced all static references to isHideTimeZone with generalSettings.privacySetting.isHideTimeZone.
• Included PrivacySettingManager in Dependency Graph as per required scope.
• Injected GeneralSettingManager throughout the app as needed.
• Updated tests to refelect above changes.

[rafaeltonholo]

Hi @shamim-emon,

Thank you for your continued contributions! We truly appreciate the effort you are putting into our app.

I have a concern regarding the direction the GeneralSettingsManager is moving towards. I believe it's important to address this now, before the interface becomes a "god interface" that manages all settings, similar to the K9 object, defeating the purpose of migrating from it.

Ideally, we should follow the Single Responsibility Principle, and I feel we are straying from that at this point.

I noticed that the current property being moved from K9 to GeneralSettingsManager pertains to privacy settings. Therefore, it would make more sense to transfer it to a settings manager specifically designed to handle privacy settings.

With that in mind, I would like to propose an alternative approach. Here’s a rough outline of my suggestion:

// NEW
// core/preference/api/src/commonMain/kotlin/net/thunderbird/core/preference/privacy/PrivacySettings.kt
data class PrivacySettings(
    val isHideTimeZone: Boolean,
)

// UPDATE
// core/preference/api/src/commonMain/kotlin/net/thunderbird/core/preference/GeneralSettings.kt
data class GeneralSettings(
    ...
    val privacy: PrivacySettings,
)

// NEW
// core/preference/api/src/commonMain/kotlin/net/thunderbird/core/preference/privacy/PrivacySettingsManager.kt
// Currently required because of how we trust in the GeneralSettings object everywhere. 
// We might be able to remove it later and only use PrivacySettingsPreferenceManager instead.
interface PrivacySettingsManager {
    val privacySettings: PrivacySettings

    fun setIsHideTimeZone(isHideTimeZone: Boolean)
}

// NEW
// core/preference/api/src/commonMain/kotlin/net/thunderbird/core/preference/privacy/PrivacySettingsPreferenceManager.kt
interface PrivacySettingsPreferenceManager : PreferenceManager<PrivacySettings>

// UPDATE
// core/preference/api/src/commonMain/kotlin/net/thunderbird/core/preference/GeneralSettingsManager.kt
interface GeneralSettingsManager : PrivacySettingsManager {
    ...
    // remove the setIsHideTimeZone function as it is part of PrivacySettingsManager
}

// NEW
// core/preference/impl/src/commonMain/kotlin/net/thunderbird/core/preference/privacy/DefaultPrivacySettingsManager.kt
class DefaultPrivacySettingsManager(
    private val preferenceManager: PrivacySettingsPreferenceManager,
) : PrivacySettingsManager {
    override val privacySettings: PrivacySettings
        get() = preferenceManager.getConfig()

    override fun setIsHideTimeZone(isHideTimeZone: Boolean) {
        val privacySettings = preferenceManager.getConfig()
        preferenceManager.save(privacySettings.copy(isHideTimeZone = isHideTimeZone))
    }
}

// NEW
// core/preference/impl/src/commonMain/kotlin/net/thunderbird/core/preference/privacy/DefaultPrivacySettingsPreferenceManager.kt
class DefaultPrivacySettingsPreferenceManager(
    private val storage: Storage,
    private val storageEditor: StorageEditor,
    private val changeBroker: PreferenceChangeBroker,
    private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
    private var scope: CoroutineScope = CoroutineScope(SupervisorJob()),
) : PrivacySettingsPreferenceManager {
    private val configState: MutableStateFlow<PrivacySettings> = MutableStateFlow(value = loadConfig())
    private val mutex = Mutex()

    init {
        asSettingsFlow()
            .onEach { config ->
                configState.update { config }
            }
            .launchIn(scope)
    }

    override fun save(config: PrivacySettings) {
        configState.update { config }
        writeConfig(config)
    }

    override fun getConfig(): PrivacySettings = configState.value
    override fun getConfigFlow(): Flow<PrivacySettings> = configState

    // Not 100% sure if this method is really required, but applied the same logic
    // present in the RealDrawerConfigManager implementation
    private fun asSettingsFlow(): Flow<PrivacySettings> {
        return callbackFlow {
            send(loadConfig())

            val subscriber = PreferenceChangeSubscriber {
                configState.update { loadConfig() }
            }

            changeBroker.subscribe(subscriber)

            awaitClose {
                changeBroker.unsubscribe(subscriber)
            }
        }
    }

    private fun loadConfig(): PrivacySettings = PrivacySettings(
        isHideTimeZone = storage.getBoolean(KEY_HIDE_TIME_ZONE, false),
    )

    private fun writeConfig(config: PrivacySettings) {
        scope.launch(ioDispatcher) {
            mutex.withLock {
                storageEditor.putBoolean(KEY_HIDE_TIME_ZONE, config.isHideTimeZone)
                storageEditor.commit()
            }
        }
    }
    
    companion object {
        private const val KEY_HIDE_TIME_ZONE = "hideTimeZone"
    }
}

// UPDATE
// legacy/core/src/main/java/com/fsck/k9/preferences/RealGeneralSettingsManager.kt
internal class RealGeneralSettingsManager(
    ...
    private val privacySettingsManager: PrivacySettingsManager,
)  : GeneralSettingsManager, PrivacySettingsManager by privacySettingsManager {
    ... other code

    // Unfortunately, since we rely on generalSettings everywhere, we need to override setIsHideTimeZone 
    // here to propagate changes within the generalSettings object. 
    // However, this can be improved later for better responsiveness.
    override fun setIsHideTimeZone(isHideTimeZone: Boolean) {
        privacySettingsManager.setIsHideTimeZone(isHideTimeZone)
        getSettings()
            .copy(privacy = privacySettings)
            .persist()
    }
}

// UPDATE
// legacy/core/src/main/java/com/fsck/k9/preferences/KoinModule.kt
val preferencesModule = module {
    ...
    single<PrivacySettingsPreferenceManager> {
        DefaultPrivacySettingsPreferenceManager(
            storage = get<Preferences>().storage,
            storageEditor = get<Preferences>().createStorageEditor(),
            changeBroker = get(),
        )
    }
    single<PrivacySettingsManager> { DefaultPrivacySettingsManager(preferenceManager = get()) }
    single {
        RealGeneralSettingsManager(
            preferences = get(),
            coroutineScope = get(named("AppCoroutineScope")),
            changePublisher = get(),
            privacySettingsManager = get(),
        )
    } bind GeneralSettingsManager::class
    ...
}

Please let me know if you want to discuss more about this matter.

Thank you for your understanding.

[rafaeltonholo]

Hi @shamim-emon, thanks for addressing the previously mentioned comments; however, there are still a few things that need to be addressed before merging this:

• Please move both DefaultPrivacySettingsManager.kt and DefaultPrivacySettingsPreferenceManager.kt to :core:preference:impl
• Revert all the formatting changes applied to HtmlQuoteCreator.java. Leave only the changes that are directly related to the introduction of the GeneralSettingsManager to that class.
• Revert all the formatting changes applied to MessageCompose.java. Leave only the changes that are directly related to the introduction of the GeneralSettingsManager to that class.
• Revert all the formatting changes applied to QuotedMessagePresenter.java. Leave only the changes that are directly related to the introduction of the GeneralSettingsManager to that class.

[rafaeltonholo]

Thanks for this contribution and for addressing all the comments!