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

Fixes New menu forgets its "Hide accounts" setting #8735

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shamim-emon
Copy link
Contributor

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.

@shamim-emon Thanks for addressing the drawer settings persistence issue. I appreciate your work on this.

I see that using K9 in this case is a quick fix, but it introduces a dependency on an outdated legacy module. Our goal is to minimize dependence on such modules, as they can harm build times, use outdated methodologies, and are subject to rewrites.

Instead, I propose moving the showAccountSelector setting into the DrawerConfig. Adding a new DrawerConfigWriter to the NavigationDrawerExternalContract and a use case SaveDrawerConfig to the DomainContract, would enable us to write tests and follow our new architecture.

I understand that dynamic updates are currently problematic, as the implementation needs to sit on top of K9. While this might look like boilerplate, it's necessary for addressing the architectural issues with K9 in the future.

As an alternative approach, I was considering a solution similar to
GeneralSettingsManager and RealGeneralSettingsManager, where changes would be exposed through a flow-based interface and values are updated via a separate call enabling a unidirectional data flow. Something like DrawerConfigManager exposing a Flow<DrawerConfig> and a save(config: DrawerConfig) which emits changes to the flow while handling persistence and sync with K9 internally.

Let me know if you have further questions.

@@ -20,6 +20,7 @@ dependencies {
implementation(projects.legacy.message)
implementation(projects.legacy.search)
implementation(projects.legacy.ui.folder)
implementation(projects.legacy.core)
Copy link
Member

Choose a reason for hiding this comment

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

Our goal is to reduce dependence on legacy code and these dependencies should be avoided where possible.

@wmontwe wmontwe self-assigned this Jan 14, 2025
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.

New menu forgets its "Hide accounts" setting
2 participants