Conversation
Contributor
📝 WalkthroughWalkthroughAdds color propagation to UI helpers: icon buttons and fast-scrolls now accept a colorInt; FastScrollerBuilder gains a colored style path; EditActivity centralizes top-action-bar color application and callers updated to pass colors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ViewGroup (layout)
participant Builder as FastScrollerBuilder
participant Resources as AppCompatResources
participant Scroll as FastScrollNestedScrollView
UI->>UI: addIconButton(title, drawable, colorInt)
UI->>UI: setControlsContrastColorForAllViews(colorInt)
UI->>Builder: addFastScroll(context, colorInt)
Builder->>Resources: load/md2_thumb_drawable (tint with colorInt)
Builder->>Scroll: attach fast-scroller (thumb + track + popup styled)
UI->>Scroll: make fast-scroll visible (styled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 1
🧹 Nitpick comments (2)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (2)
701-701: Remove unused variable.The
countervariable appears to be dead code that serves no purpose.🔧 Suggested fix
-var counter = 0 -🤖 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/UiExtensions.kt` at line 701, Remove the unused top-level variable `counter` declared in UiExtensions.kt (the `var counter = 0` line); search for any references to `counter` to confirm it's unused and then delete the declaration (or replace with the intended implementation if it was meant to be used), ensuring no remaining dead code or unused imports remain in the file.
1001-1015: Inconsistent null handling for drawables.Lines 1005-1010 safely handle a potentially null drawable with
?.let, but line 1011 uses a non-null assertion (!!) onContextCompat.getDrawable. For consistency and defensive coding, consider using the same safe pattern.🛡️ Suggested fix
- setTrackDrawable(ContextCompat.getDrawable(context, R.drawable.scroll_track)!!) + ContextCompat.getDrawable(context, R.drawable.scroll_track)?.let { setTrackDrawable(it) }🤖 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/UiExtensions.kt` around lines 1001 - 1015, The useColoredStyle extension inconsistently force-unwraps the track drawable with ContextCompat.getDrawable(... )!!; change it to handle null defensively like the thumb drawable: retrieve the track with ContextCompat.getDrawable(context, R.drawable.scroll_track) and only call setTrackDrawable(...) when the result is non-null (e.g., using ?.let) or provide a clear fallback drawable/resource before calling setTrackDrawable; update FastScrollerBuilder.useColoredStyle to use that safe pattern.
🤖 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/UiExtensions.kt`:
- Around line 726-728: Remove the unnecessary API check around
FastScrollNestedScrollView (delete the Build.VERSION.SDK_INT >= Q gating) and
prevent repeated FastScrollerBuilder rebuilds by making addFastScroll
idempotent: detect if a fast scroller is already installed (e.g., store/reuse a
builder or install marker via view.setTag or a weak map keyed by
FastScrollNestedScrollView) and skip creating/building a new FastScrollerBuilder
on subsequent calls; update setControlsColorForAllViews to call addFastScroll
without causing repeated rebuilds. Also fix useColoredStyle’s inconsistent
null-safety by replacing the non-null assertion on the track drawable with a
safe call and conditional handling (mirror the thumb drawable’s ?.let pattern)
to avoid potential crashes when drawables are missing.
---
Nitpick comments:
In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt`:
- Line 701: Remove the unused top-level variable `counter` declared in
UiExtensions.kt (the `var counter = 0` line); search for any references to
`counter` to confirm it's unused and then delete the declaration (or replace
with the intended implementation if it was meant to be used), ensuring no
remaining dead code or unused imports remain in the file.
- Around line 1001-1015: The useColoredStyle extension inconsistently
force-unwraps the track drawable with ContextCompat.getDrawable(... )!!; change
it to handle null defensively like the thumb drawable: retrieve the track with
ContextCompat.getDrawable(context, R.drawable.scroll_track) and only call
setTrackDrawable(...) when the result is non-null (e.g., using ?.let) or provide
a clear fallback drawable/resource before calling setTrackDrawable; update
FastScrollerBuilder.useColoredStyle to use that safe pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 935915e2-a12f-406d-bb04-8c8e38cb9477
📒 Files selected for processing (4)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditNoteActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditTextPlainActivity.kt
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt
Outdated
Show resolved
Hide resolved
Crustack
changed the title
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt (1)
1011-1013:⚠️ Potential issue | 🟠 MajorMake fast-scroll installation idempotent.
setControlsColorForAllViews()can hit this helper multiple times for the sameFastScrollNestedScrollView, and this implementation always creates a newFastScrollerBuilder(...).build(). That can stack multiple fast scrollers/listeners when note colors are reapplied.Run this to confirm the recursive recolor path reaches an unguarded
build()call:#!/bin/bash sed -n '704,729p' app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt printf '\n---\n' sed -n '1011,1029p' app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt🤖 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/UiExtensions.kt` around lines 1011 - 1013, The addFastScroll helper creates a new FastScroller every time and can be invoked repeatedly by setControlsColorForAllViews, stacking listeners; modify addFastScroll(ViewGroup.addFastScroll) to be idempotent by checking for and returning early if a marker is present (e.g., view.getTag(KEY_FAST_SCROLL_INSTALLED) or a boolean property on the FastScrollNestedScrollView child) before calling FastScrollerBuilder(...).useColoredStyle(...).build(), and after a successful build set that same marker on the ViewGroup (or on the FastScrollNestedScrollView child) so subsequent calls skip building again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.kt`:
- Around line 1011-1013: The addFastScroll helper creates a new FastScroller
every time and can be invoked repeatedly by setControlsColorForAllViews,
stacking listeners; modify addFastScroll(ViewGroup.addFastScroll) to be
idempotent by checking for and returning early if a marker is present (e.g.,
view.getTag(KEY_FAST_SCROLL_INSTALLED) or a boolean property on the
FastScrollNestedScrollView child) before calling
FastScrollerBuilder(...).useColoredStyle(...).build(), and after a successful
build set that same marker on the ViewGroup (or on the
FastScrollNestedScrollView child) so subsequent calls skip building again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b378ffb6-69d9-4ce8-b86f-85ae0a6af725
📒 Files selected for processing (3)
app/src/main/java/com/philkes/notallyx/presentation/UiExtensions.ktapp/src/main/java/com/philkes/notallyx/presentation/activity/note/EditActivity.ktapp/src/main/java/com/philkes/notallyx/presentation/view/main/BaseNoteVH.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.
Fixes #901
Summary by CodeRabbit