Convert ms timeouts to seconds in UI#225
Conversation
Display BacklightTimeoutMs and NotificationTimeoutMs as seconds and minutes:seconds respectively. Adjusts slider steps and formats default values to match the new units.
|
Justin Sunseri seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
cc @ericmigi |
| ) | ||
| return when (pref) { | ||
| NumberWatchPref.BacklightTimeoutMs -> { | ||
| val secValue = item.valueOrDefault() / 1000 |
There was a problem hiding this comment.
Prefer using kotlin duration to do all the maths here e.g. item.valueOrDefault().milliseconds.inWholeSeconds
There was a problem hiding this comment.
should be resolved
| topLevelType = TopLevelType.Watch, | ||
| section = pref.section(), | ||
| value = secValue, | ||
| min = 1, |
There was a problem hiding this comment.
min/are already defined in the NumberWatchPref enum - shouldn't be defining them again separately (e.g. use pref.min.milliseconds.inWholeSeconds here
There was a problem hiding this comment.
should be resolved
| min = 1, | ||
| max = 10, | ||
| onValueChange = { | ||
| libPebble.setWatchPref(item.copy(value = it * 1000)) |
There was a problem hiding this comment.
Same for all of these - use kotlin time functions, not / 1000, for better code clarity
There was a problem hiding this comment.
should be resolved
| valueFormatter = { v -> | ||
| "${v / 60}:${(v % 60).toString().padStart(2, '0')}" | ||
| }, | ||
| stepsOverride = 19, |
| isDebugSetting: Boolean = false, | ||
| defaultValue: Long? = null, | ||
| valueFormatter: ((Long) -> String)? = null, | ||
| stepsOverride: Int? = null, |
| valueFormatter = { "$it seconds" }, | ||
| ) | ||
| } | ||
| NumberWatchPref.NotificationTimeoutMs -> { |
There was a problem hiding this comment.
This is kind of the same as the one above but with a different formatter - worth splitting out into a basicSettingsNumberSecondsItem and passing in through a different formatter?
There was a problem hiding this comment.
I believe this has been addressed. thanks for the comment
| topLevelType = TopLevelType.Watch, | ||
| section = pref.section(), | ||
| value = secValue, | ||
| min = 0, |
There was a problem hiding this comment.
same as above - don't repeat the defaults here (they'll get out of sync if we ever change them)
| val minF = remember(min) { min.toFloat() } | ||
| val maxF = remember(max) { max.toFloat() } | ||
| val steps = remember(max, min) { | ||
| val steps = stepsOverride ?: remember(max, min) { |
There was a problem hiding this comment.
oh this is actually setting the number of steps... not test code!
There was a problem hiding this comment.
yup. i renamed this a bit so it reads a little bit better to me though.
|
@sjp4 I know these edit's compile but I'm traveling for work and my personal laptop that i do hobby dev on is so under powered that i can barely compile the mobile app and the emulator crashes because of some incompatibility in the graphics drivers (2013 MacBook Air running Fedora). Could you please try this out locally? If not it will have to wait till i get back from my trip. |
3 participants
Display BacklightTimeoutMs and NotificationTimeoutMs as seconds and minutes:seconds respectively. Adjusts slider steps and formats default values to match the new units.