-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
base: main
Are you sure you want to change the base?
Conversation
shamim-emon
commented
Jan 11, 2025
- Fixes New menu forgets its "Hide accounts" setting #8709
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted!
@wmontwe I see the concern with this approach. I will move forward with your suggestion. I don't have any question at the moment but if anytime comes up, I will be sure to ask. Thank you! |