Replace setting sliders with steppers where useful#963
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces seekbar-based integer preference controls with a new stepper control (buttons + numeric input), adds stepper binding helpers with clamping, auto-repeat and keyboard handling, updates SettingsFragment wiring, raises several IntPreference upper bounds, and adds layout, drawable, style, and string resources for the stepper UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`:
- Around line 524-541: Clamp the incoming preference value before binding so
stale/out-of-range ints can't lock the stepper: compute a clampedValue =
value.coerceIn(min, max) and use clampedValue for initial
ValueInput.setText(...), ValueInput.tag, the call to
updateButtons(clampedValue), and as the fallback inside updateValue (instead of
raw value); update the same pattern in the other occurrence (lines ~598-623).
Ensure MinusButton/PlusButton enabling, displayed text, and tag all derive from
clampedValue.
- Around line 559-596: The touch listeners call onChange on ACTION_UP which
duplicates the click listener's save; to fix, add a small boolean flag (e.g.,
handledClick or isAutoIncrementing) and update it from the click/long-click
handlers so the touch listener only calls onChange if the click wasn't already
handled. Specifically, in the PlusButton/MinusButton click lambdas (where
updateValue(…) is called) set handledClick = true, in the touch listener check
if handledClick is false before calling onChange(ValueInput.tag as? Int ?:
value), and always reset handledClick = false at the end of the touch handling
(or when starting/stopping auto-increment in startAutoIncrement/runnable) so
subsequent interactions behave normally.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Around line 382-385: Replace the hardcoded English label logic in the
labelFormatter lambdas with Android plurals: when v == 0 return the existing off
string (requireContext().getString(R.string.off)) and for v > 0 return
requireContext().resources.getQuantityString(R.plurals.days, v, v); update both
occurrences of labelFormatter in SettingsFragment (the one around the 382 block
and the one around 778–780) and add/update the R.plurals.days resource
(singular/plural forms) so localization produces "1 day" vs "2 days" correctly.
In `@app/src/main/res/layout/preference_stepper.xml`:
- Around line 10-15: The stepper lacks accessibility labels: link the TextView
with id "Title" to the input by adding android:labelFor pointing at the input
view (e.g., "ValueInput"), and ensure both icon-only buttons have descriptive
android:contentDescription values (use string resources like
"increment_quantity" and "decrement_quantity"); apply the same fixes to the
other stepper blocks referenced (lines 24-34, 46-59, 63-73) so TalkBack users
can identify the setting and which button increments vs decrements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe60d503-853a-498e-a221-766a96d9867b
⛔ Files ignored due to path filters (1)
app/translations.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (8)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.ktapp/src/main/java/com/philkes/notallyx/presentation/viewmodel/preference/NotallyXPreferences.ktapp/src/main/res/drawable/remove.xmlapp/src/main/res/layout/fragment_settings.xmlapp/src/main/res/layout/preference_stepper.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/values/themes.xml
Comment on lines
+559
to
+596
| var runnable: Runnable? = null | ||
| val startAutoIncrement = { isIncrement: Boolean -> | ||
| runnable?.let { handler.removeCallbacks(it) } | ||
| runnable = | ||
| if (isIncrement) stepperRunnable(isIncrement = true) | ||
| else stepperRunnable(isIncrement = false) | ||
| handler.postDelayed(runnable!!, 100) | ||
| } | ||
|
|
||
| MinusButton.setOnClickListener { updateValue(-1, true) } | ||
|
|
||
| MinusButton.setOnLongClickListener { | ||
| startAutoIncrement(false) | ||
| true | ||
| } | ||
|
|
||
| MinusButton.setOnTouchListener { _, event -> | ||
| if (event.action == MotionEvent.ACTION_UP || event.action == MotionEvent.ACTION_CANCEL) { | ||
| runnable?.let { handler.removeCallbacks(it) } | ||
| onChange(ValueInput.tag as? Int ?: value) | ||
| } | ||
| false | ||
| } | ||
|
|
||
| PlusButton.setOnClickListener { updateValue(1, true) } | ||
|
|
||
| PlusButton.setOnLongClickListener { | ||
| startAutoIncrement(true) | ||
| true | ||
| } | ||
|
|
||
| PlusButton.setOnTouchListener { _, event -> | ||
| if (event.action == MotionEvent.ACTION_UP || event.action == MotionEvent.ACTION_CANCEL) { | ||
| runnable?.let { handler.removeCallbacks(it) } | ||
| onChange(ValueInput.tag as? Int ?: value) | ||
| } | ||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
Don't commit twice on a normal tap.
The touch listener saves on every button release, and the click listener saves again immediately after. For settings with side effects, that means duplicated work on every tap; AutoEmptyBin now cancels/schedules WorkManager twice.
Suggested fix
- fun stepperRunnable(isIncrement: Boolean) =
+ var didAutoRepeat = false
+ fun stepperRunnable(isIncrement: Boolean) =
object : Runnable {
override fun run() {
if (updateValue(if (isIncrement) 1 else -1, false)) {
+ didAutoRepeat = true
handler.postDelayed(this, 100)
}
}
}
var runnable: Runnable? = null
val startAutoIncrement = { isIncrement: Boolean ->
+ didAutoRepeat = false
runnable?.let { handler.removeCallbacks(it) }
runnable =
if (isIncrement) stepperRunnable(isIncrement = true)
else stepperRunnable(isIncrement = false)
handler.postDelayed(runnable!!, 100)
@@
MinusButton.setOnTouchListener { _, event ->
if (event.action == MotionEvent.ACTION_UP || event.action == MotionEvent.ACTION_CANCEL) {
runnable?.let { handler.removeCallbacks(it) }
- onChange(ValueInput.tag as? Int ?: value)
+ if (didAutoRepeat) {
+ didAutoRepeat = false
+ onChange(ValueInput.tag as? Int ?: value)
+ }
}
false
}
@@
PlusButton.setOnTouchListener { _, event ->
if (event.action == MotionEvent.ACTION_UP || event.action == MotionEvent.ACTION_CANCEL) {
runnable?.let { handler.removeCallbacks(it) }
- onChange(ValueInput.tag as? Int ?: value)
+ if (didAutoRepeat) {
+ didAutoRepeat = false
+ onChange(ValueInput.tag as? Int ?: value)
+ }
}
false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`
around lines 559 - 596, The touch listeners call onChange on ACTION_UP which
duplicates the click listener's save; to fix, add a small boolean flag (e.g.,
handledClick or isAutoIncrementing) and update it from the click/long-click
handlers so the touch listener only calls onChange if the click wasn't already
handled. Specifically, in the PlusButton/MinusButton click lambdas (where
updateValue(…) is called) set handledClick = true, in the touch listener check
if handledClick is false before calling onChange(ValueInput.tag as? Int ?:
value), and always reset handledClick = false at the end of the touch handling
(or when starting/stopping auto-increment in startAutoIncrement/runnable) so
subsequent interactions behave normally.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (1)
383-385:⚠️ Potential issue | 🟡 MinorUse localized plurals instead of hardcoded unit suffixes.
Both formatters currently build English strings directly (
"$v ..."/"${v}s"), which breaks singular/plural grammar and localization. Please switch both to quantity resources (getQuantityString) and keep only the sentinel “off” branch special-cased.Also applies to: 780-782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt` around lines 383 - 385, The labelFormatter lambdas in SettingsFragment (the anonymous lambda assigned to labelFormatter) currently build English strings directly; replace those string interpolations with Android quantity resources by calling requireContext().resources.getQuantityString(R.plurals.<name>, v, v) for the non-zero branch and keep the v == 0 branch returning requireContext().getString(R.string.off); add appropriate plurals entries in R.plurals (e.g., days, hours) and use those plural resource names in both formatter locations (the shown labelFormatter and the other formatter around lines 780-782) so singular/plural and localization are handled correctly.app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt (2)
560-567:⚠️ Potential issue | 🟠 MajorReset long-press state after release to avoid unintended extra commits.
runnableis never nulled afterACTION_UP/ACTION_CANCEL. After one long-press interaction, later taps can still enter the touch commit path becauserunnable != null, causing duplicate saves.Proposed fix
if ( runnable != null && (event.action == MotionEvent.ACTION_UP || event.action == MotionEvent.ACTION_CANCEL) ) { handler.removeCallbacks(runnable!!) + runnable = null onChange(ValueInput.tag as? Int ?: initialValue) }Also applies to: 575-585, 594-604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt` around lines 560 - 567, The long-press Runnable (runnable) created by startAutoIncrement/stepperRunnable is never cleared on ACTION_UP/ACTION_CANCEL so subsequent taps treat runnable != null and trigger duplicate commits; update the touch/gesture cleanup code (the touch event handlers that call startAutoIncrement/handler.postDelayed) to call handler.removeCallbacks(runnable) and set runnable = null on ACTION_UP and ACTION_CANCEL (and any early exit paths), ensuring the Runnable is cancelled and the reference cleared after release to prevent re-entering the long-press commit path.
582-583:⚠️ Potential issue | 🟠 MajorUse the clamped initial value consistently for
ValueInput.tagand commit fallbacks.Line 608 sets
tagfrom rawvalue, which can be out of bounds and make stepping get stuck for stale/imported values. The touch fallback paths also use rawvalue. Initialize and fallback withinitialValueinstead.Proposed fix
- onChange(ValueInput.tag as? Int ?: value) + onChange(ValueInput.tag as? Int ?: initialValue) ... - onChange(ValueInput.tag as? Int ?: value) + onChange(ValueInput.tag as? Int ?: initialValue) ... - tag = value + tag = initialValueAlso applies to: 601-602, 608-608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt` around lines 582 - 583, The code currently assigns and falls back to the raw value, which can be out of bounds; instead compute and use the clamped initialValue everywhere: set ValueInput.tag = initialValue (not raw value), call onChange(initialValue), and update any touch-fallback and commit-fallback paths to use initialValue as the fallback. Locate usages around ValueInput.tag, onChange and the touch/commit fallback branches and replace raw value fallbacks with the computed initialValue so stepping won't get stuck on stale/imported values.
🧹 Nitpick comments (1)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt (1)
16-16: Remove or gate temporary debug logging in this save path.
Log.d("Stepper", "save auto remove")looks like leftover debug instrumentation. It should be removed or guarded behind a debug flag to avoid noisy production logs.Suggested cleanup
-import android.util.Log ... - Log.d("Stepper", "save auto remove")Also applies to: 388-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt` at line 16, The debug statement Log.d("Stepper", "save auto remove") in SettingsFragment is leftover instrumentation and should be removed or gated; either delete that call (and the duplicate at the other occurrence) or wrap it in a debug-only guard such as if (BuildConfig.DEBUG) { Log.d("Stepper", "save auto remove") } (or replace with a production-safe logger like Timber.tag("Stepper").d(...) behind a debug flag) so stray debug logs do not appear in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`:
- Around line 537-549: The updateValue(increment: Int, commit: Boolean) method
calls onChange(newValue) and then clears focus, which triggers the focus-loss
handler that also commits—causing duplicate commits; to fix, add a short-lived
suppression flag (e.g., suppressNextFocusCommit) on the enclosing
view/controller (or attach to ValueInput via tag) that updateValue sets true
just before calling onChange when commit==true and clears after clearing focus
(or the focus-loss handler clears it), and modify the focus-loss handler to
check this flag and skip calling onChange if suppression is set; update
references: updateValue, ValueInput, onChange, and the focus-loss handler so the
handler returns early when suppression is active.
---
Duplicate comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.kt`:
- Around line 560-567: The long-press Runnable (runnable) created by
startAutoIncrement/stepperRunnable is never cleared on ACTION_UP/ACTION_CANCEL
so subsequent taps treat runnable != null and trigger duplicate commits; update
the touch/gesture cleanup code (the touch event handlers that call
startAutoIncrement/handler.postDelayed) to call
handler.removeCallbacks(runnable) and set runnable = null on ACTION_UP and
ACTION_CANCEL (and any early exit paths), ensuring the Runnable is cancelled and
the reference cleared after release to prevent re-entering the long-press commit
path.
- Around line 582-583: The code currently assigns and falls back to the raw
value, which can be out of bounds; instead compute and use the clamped
initialValue everywhere: set ValueInput.tag = initialValue (not raw value), call
onChange(initialValue), and update any touch-fallback and commit-fallback paths
to use initialValue as the fallback. Locate usages around ValueInput.tag,
onChange and the touch/commit fallback branches and replace raw value fallbacks
with the computed initialValue so stepping won't get stuck on stale/imported
values.
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Around line 383-385: The labelFormatter lambdas in SettingsFragment (the
anonymous lambda assigned to labelFormatter) currently build English strings
directly; replace those string interpolations with Android quantity resources by
calling requireContext().resources.getQuantityString(R.plurals.<name>, v, v) for
the non-zero branch and keep the v == 0 branch returning
requireContext().getString(R.string.off); add appropriate plurals entries in
R.plurals (e.g., days, hours) and use those plural resource names in both
formatter locations (the shown labelFormatter and the other formatter around
lines 780-782) so singular/plural and localization are handled correctly.
---
Nitpick comments:
In
`@app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt`:
- Line 16: The debug statement Log.d("Stepper", "save auto remove") in
SettingsFragment is leftover instrumentation and should be removed or gated;
either delete that call (and the duplicate at the other occurrence) or wrap it
in a debug-only guard such as if (BuildConfig.DEBUG) { Log.d("Stepper", "save
auto remove") } (or replace with a production-safe logger like
Timber.tag("Stepper").d(...) behind a debug flag) so stray debug logs do not
appear in production.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec57411d-2363-4e0a-91c4-5b20aa735814
📒 Files selected for processing (2)
app/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/PreferenceBindingExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/main/fragment/settings/SettingsFragment.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #924
notallyx_issues_settings_stepper.webm
Summary by CodeRabbit
New Features
Bug Fixes / Improvements