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

Add mechanism to listen for general settings changes #8763

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cketti
Copy link
Member

@cketti cketti commented Jan 21, 2025

Use that mechanism to update the drawer when any of the DrawerConfig settings are changed.

Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

I think this is solving the callaback issue, but ties the implementation to GeneralSettingsManager exclusively. I think it would make sense to extract the notification logic and make that available for use in feature configurations too. So it plays nicely with #8735

Comment on lines +160 to +173

override fun addListener(listener: GeneralSettingsChangeListener) {
listeners.add(listener)
}

override fun removeListener(listener: GeneralSettingsChangeListener) {
listeners.remove(listener)
}

private fun notifyListeners() {
for (listener in listeners) {
listener.onGeneralSettingsChanged()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to extract this into it's own class to allow reuse in feature configurations to avoid the need to always depend on GeneralSettingsManager when interested in change notifications.

@@ -0,0 +1,5 @@
package app.k9mail.legacy.preferences

fun interface GeneralSettingsChangeListener {
Copy link
Member

Choose a reason for hiding this comment

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

This could be more generic like SettingsChangeListener and onSettingsChanged()

Comment on lines +19 to +21

fun addListener(listener: GeneralSettingsChangeListener)
fun removeListener(listener: GeneralSettingsChangeListener)
Copy link
Member

Choose a reason for hiding this comment

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

See above, could be part of a specialized class to handle listener registration.

@cketti
Copy link
Member Author

cketti commented Feb 6, 2025

This is meant to be a small improvement to the current situation, not a proper solution to the settings problem. Right now there's no way for feature modules to retrieve general settings without depending on either K9 or GeneralSettingsManager (or both). I don't think extracting the listener part to a separate class will improve the situation.

For a proper solution I think we will want to have a low-level settings store that feature modules can use to store and retrieve their settings (both general and account settings). Once we have that, I think it makes sense for this settings store to also contain the listener mechanism and not have it be separate. I don't see a situation where some code wants to be notified about settings changes, but not also retrieve the new settings.

interface SettingsStore {
    fun getInt(key: String): Int
    fun setInt(key: String, value: Int)

    fun getString(key: String): String
    fun setString(key: String, value: String)

    //

    fun addListener(listener: SettingsChangeListener)
    fun removeListener(listener: SettingsChangeListener)

    // Probably also a mechanism to change more than one setting at once
}

However, that's a much more complex change that requires deeper architectural changes. I think this PR fixes an immediate problem and only introduces code that should be fairly straight-forward to adapt to a proper solution in the future.


Note: I added the listening mechanism to GetDrawerConfig because it contained the flow() call. But looking at it again, I think it should be moved to DrawerConfigLoader (that probably should be renamed to DrawerConfigProvider?). What do you think?

@wmontwe
Copy link
Member

wmontwe commented Feb 6, 2025

I thought about implementing a Publish-Subscribe (Pub/Sub) pattern to decouple the producer and consumer of settings change events, enabling individual registration of feature configurations.
This is a low level change that doesn't require large rewrites or architectural changes. Additionally, it offers the potential to be extended to more specific change events if ever needed.

I'll create a pull-request for this.


Yes, it should be part of the DrawerConfigLoader. I'm not sure about the name and maybe this code should be moved to app-common to avoid duplication between the apps, hence there is no need for specific behaviour. But currently app-common is not yet setup properly.

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.

2 participants