Implement a ThemeSwitcher to alternate between dark and light mode #16
Conversation
fmontesino
assigned kleinsenberg, geftimov and KaneCheshire and unassigned kleinsenberg, geftimov and KaneCheshire
app/src/main/java/com/kinandcarta/create/proxytoggle/android/ThemeSwitcher.kt
Outdated
Show resolved
Hide resolved
| @@ -14,7 +15,8 @@ import com.kinandcarta.create.proxytoggle.settings.AppSettings | |||
| class ProxyManagerViewModel @ViewModelInject constructor( | |||
| private val deviceSettingsManager: DeviceSettingsManager, | |||
| private val proxyValidator: ProxyValidator, | |||
| private val appSettings: AppSettings | |||
| private val appSettings: AppSettings, | |||
| private val themeSwitcher: ThemeSwitcher | |||
There was a problem hiding this comment.
so this class is acting as a repository and a reference/controller of the ui?
There was a problem hiding this comment.
Well, this class collects the click actions from the view (and acts accordingly) and also exposes the data that the UI needs to show. I don't think I'd call it a Repository unless you have in mind another definition for what a Repository means?
There was a problem hiding this comment.
AppSettings is a data source - Repositories are classes or components that encapsulate the logic required to access data sources... ThemeSwitcher seems to directly control the view - instead of having the view model provide it with actions and data/state, which is what a view model is responsible of doing. That's why this class looks like a view controller and repository to me
There was a problem hiding this comment.
Not sure I follow your point.
Are you suggesting the ViewModel should expose a state based on the ThemeSwitcher's value, and then the view should change the theme itself?
Also, I am using AppSettings interface to expose the last used proxy. If I create a Repository, it will only be for a setter/getter of one value - and it will never grow.
Given the size of this app (and the fact that it won't grow more than this) and given that the AppSettings implementation has its own setters/getters from the SharedPrefs, do you think it's necessary creating a Repository for just this?
There was a problem hiding this comment.
Not saying anything is necessary... I was just trying to understand the role of this class, as is it isn't a view model
There was a problem hiding this comment.
If you make ThemeSwitcher an interface then I would say this class is somewhat a Presenter in MVP
Can definitely do that!
Although, aren't the ViewModel in MVVM and the Presenter in MVP a similar piece of the architecture, in terms of responsibilities? They have obvious differences when it comes to implementation, but they are still the same thing for the View
There was a problem hiding this comment.
I didn't say you need to change something. You could even just write everything in one class if this is going to be so small. No need for clean architecture
This class isn't triggering events though, it is calling them directly, for example themeSwitcher.toggleTheme(). A view model to be a view model would be called by the view or receive some data from a data source and then emit a new state that will trigger the change in the view. Also looking at ThemeSwitcher it is also directly calling a data source
There was a problem hiding this comment.
There's lots of material on the web about what a view model is in MVVM, including official documentation. MVVM, MVP, MVC, MVI can be applied on any system with a view from Android, Web and iOS. They didn't originate from Android. This Stackoverflow post is one of the shorter resource shared at companies I have worked at to get a better understanding of MVVM https://stackoverflow.com/questions/5421874/basic-concepts-of-mvvm-what-should-a-viewmodel-do
There was a problem hiding this comment.
If you have some other official definition of a view model in MVVM specifically for Android, please feel free to pm me and share. I am happy to learn :)
There was a problem hiding this comment.
As I said, let's take this offline and we can raise a separate PR for addressing the architecture/naming convention :)
Why?
We want to give the users the option to switch between dark and light theme
What?
Added a ThemeSwitcher that will toggle between dark and light theme, storing the preference for future uses.
Whenever the app is launched, it will first check if there's a stored setting and will apply automatically.
If there's no stored setting, it will check the device configuration. If the configuration is set to dark, it will apply the dark mode automatically.
Bonus
Here some screenshots (dark theme is not yet implemented!)
