feat: start settings page #37
Conversation
composeApp/src/commonMain/kotlin/org/ooni/probe/CreateDataStore.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/di/Dependencies.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The only blocking comments are regarding the shared preference names, which should match the previous app (I know our applicationIds are still different, but we'll need to match them as well in the future).
The bigger architecture suggestions could be made in the future, but I think it's an important discussion to have.
composeApp/src/commonMain/kotlin/org/ooni/probe/ui/settings/SettingsScreen.kt
Outdated
Show resolved
Hide resolved
composeApp/src/androidMain/kotlin/org/ooni/probe/AndroidApplication.kt
Outdated
Show resolved
Hide resolved
composeApp/src/androidUnitTest/kotlin/org/ooni/testing/CreatePreferenceDataStore.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/ui/navigation/Navigation.kt
Outdated
Show resolved
Hide resolved
| return "${prefix?.let { "${it}_" } ?: ""}$name${if (autoRun) "_autorun" else ""}" | ||
| } | ||
|
|
||
| fun allSettings(keys: List<Preferences.Key<*>>): Flow<Map<String, Any?>> = |
There was a problem hiding this comment.
This abstraction seems a bit off, but I want to get your feedback on it. We have an enum with all the keys: SettingsKey. But we're not using it in all these methods. We could be doing a allSettings(keys: List<SettingsKey>) for example.
On the other side, it's the responsibility of the users of this class to call the datastore methods like booleanPreferencesKey to actually create the key. Should this class be hiding that? Or maybe a domain class in between to abstract it? In theory we want repositories, or at least the domain layer, to hide implementation details such as DataStore.
composeApp/src/androidUnitTest/kotlin/org/ooni/testing/CreatePreferenceDataStore.kt
Outdated
Show resolved
Hide resolved
composeApp/src/iosMain/kotlin/org/ooni/probe/SetupDependencies.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonTest/kotlin/org/ooni/probe/data/repositories/PreferenceRepositoryTest.kt
Outdated
Show resolved
Hide resolved
| import platform.Foundation.NSURL | ||
| import platform.Foundation.NSUserDomainMask | ||
|
|
||
| @OptIn(kotlinx.cinterop.ExperimentalForeignApi::class) |
There was a problem hiding this comment.
This is probably not needed anymore.
| _state.update { state -> | ||
| state.copy( | ||
| preference = state.preference?.plus(it.key to it.value), | ||
| ) | ||
| } |
There was a problem hiding this comment.
You shouldn't need to do this. The state should update automatically since you subscribed above to the flow from the preferencesManager.allSettings.
| (() -> Unit)? = null, | ||
| ) | ||
|
|
||
| data class SettingsItem( |
There was a problem hiding this comment.
I feel this classes should be in separate files, especially because they are not just related with this screen.
| return "${prefix?.let { "${it}_" } ?: ""}$name${if (autoRun) "_autorun" else ""}" | ||
| } | ||
|
|
||
| fun preferenceKeyFromSettingsKey( |
There was a problem hiding this comment.
If getValueByKey and setValueByKey would use SettingsKey as well, this method could be private.
Closes #26
Proposed changes.
Sample UI