font scale setting screen #6453
Conversation
| fun getSystemFontScale(): Float | ||
| } | ||
|
|
||
| class SystemSettingsProviderImpl @Inject constructor( |
There was a problem hiding this comment.
rather than Impl we could provide more context here by renaming the implementation AndroidSystemSettingsProvider, for other cases where the interface and implementation are the same (extracted for testing or design reasons) the convention is to use Default{Interface}
There was a problem hiding this comment.
I don't really think there is any other system rather than Android we could have, so System and AndroidSystem are quite same for me. And since so, I'll rename to AndroidSystemSettingsProvider :)
| import javax.inject.Inject | ||
|
|
||
| /** | ||
| * A helper to get system settings. |
There was a problem hiding this comment.
NIT: helper is one of those generic terms that doesn't mean much, everything can be a helper/util 😄
it also asks the questions if we need to leak the knowledge of the SystemSettings, does the consumer of getSystemFontScale benefit from knowing it's a System setting, would other Systems provide that information in the same way? 🤔
this might be more flexible if it was coupled to the font domain rather than SystemSettings, eg FontSettingsProvider with an AndroidSystemSettingsProvider providing the implementation
what do you think?
There was a problem hiding this comment.
does the consumer of getSystemFontScale benefit from knowing it's a System setting
Yes, the main purpose of this class is to provide system settings to be compared with app settings or used instead of it. So there is a clear and significant difference between system setting and app setting for consumer of this class
There was a problem hiding this comment.
this might be more flexible if it was coupled to the font domain rather than SystemSettings, eg FontSettingsProvider
yes, it would be more flexible, but that's also a level of abstraction which we don't need and most likely we won't need in a future. There are no other system settings we provide for now and adding another class for font domain will just increase number of classes involved in resolving this setting, without giving us any real benefit from it's flexibility
| } | ||
|
|
||
| @Test | ||
| fun `when handling FontScaleChangedAction, then changes state and emits RestartActivity event`() = runTest { |
There was a problem hiding this comment.
a convention commonly used by gherkin style unit tests is to use line breaks as a way to signify the different parts of the test
// given
setup parameters and dependencies
// when
call method under test
// then
assert result
|
|
||
| @Test | ||
| fun `when handling FontScaleChangedAction, then changes state and emits RestartActivity event`() = runTest { | ||
| val scaleOptions = fakeFontScalePreferences.getAvailableScales() |
There was a problem hiding this comment.
ideally our tests would only make real calls to the method currently under test, which for this test case is ViewModel.handle
by calling methods on other classes we're coupling our tests to them, if FontScalePreferences.getAvailableScales were to change, this test would also need to be updated
something else to bear in mind is which information is needed by the test, for this test case we want to know if given a list of options, when selecting one, is the state updated, restart emitted and the selection persisted, which means we can simplify to...
// A reusable fixture for the given data
private val A_SELECTION = aFontScaleValue(index = 1)
// the ViewModel tests don't need to know about the real font scale options
private val A_SCALE_OPTIONS_WITH_SELECTION = listOf(
aFontScaleValue(index = 0),
A_SELECTION,
)
@Test
fun `given scale options containing selection, when handling FontScaleChangedAction with selection, then persists selection, updates selected index and emits RestartActivity`() {
// we setup the fake with our static test data related to the test name
fakeFontScalePreferences.givenAvailableScaleOptions(A_SCALE_OPTIONS_WITH_SELECTION)
viewModelWith(initialState)
val test = viewModel.test()
viewModel.handle(FontScaleSettingAction.FontScaleChangedAction(A_SELECTION))
test
.assertStatesChanges(
// the ViewModel mutates the initial state in its `init`
initialState.copy(availableScaleOptions = A_SCALE_OPTIONS_WITH_SELECTION),
{ copy(persistedSettingIndex = A_SELECTION.index) }
)
.assertEvents(FontScaleSettingViewEvents.RestartActivity)
.finish()
fakeFontScalePreferences.verifyAppScaleFontValue(A_SELECTION)
}
// our tests only make use of the index
fun aFontScaleValue(index: Int) = FontScaleValue(index, "foo", -1f, 0)
| /** | ||
| * Object to manage the Font Scale choice of the user. | ||
| */ | ||
| @Singleton |
There was a problem hiding this comment.
what's the reason for using @Singleton? I would lean against the usage as the class isn't heavy to instantiate and doesn't directly hold any mutable state
once the classloader encounters a singleton it (and its dependencies dependencies) will be in memory for the lifetime of the application (this class wouldn't be needed once the app is in the background for example)
|
SonarCloud Quality Gate failed. |
| @Provides | ||
| @Singleton | ||
| @DefaultPreferences | ||
| fun providesDefaultSharedPreferences(context: Context): SharedPreferences { |
Type of change
Content
Screen, where user can select font scaling specifically for element app, or choose to use system-wide setting
Motivation and context
#5687
Screenshots / GIFs
Tests
Tested devices
Checklist