ADFA-3263 | Refactor FeedbackButtonManager and use position ratios#1086
Merged
ADFA-3263 | Refactor FeedbackButtonManager and use position ratios#1086
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 Walkthrough
Improvements:
Risks / best-practice notes:
Testing recommendations:
WalkthroughAdds modular draggable FAB support: a touch listener for drag/long-press, a calculator to compute safe bounds and ratio conversions, a repository to persist FAB position ratios, and a refactor of FeedbackButtonManager to use these components and instance-based lifecycle. Changes
Sequence DiagramsequenceDiagram
participant User as User Touch
participant Listener as DraggableTouchListener
participant Calc as FabPositionCalculator
participant Repo as FabPositionRepository
participant FAB as FloatingActionButton
participant Prefs as SharedPreferences
User->>Listener: ACTION_DOWN (capture start coords)
User->>Listener: ACTION_MOVE (exceeds touchSlop)
Listener->>Calc: getSafeDraggingBounds(parent, fab)
Calc-->>Listener: safe bounds Rect
Listener->>FAB: update x/y (coerceIn bounds)
User->>Listener: ACTION_UP
Listener->>Calc: validateAndCorrectPosition(x,y,parent,fab)
Calc-->>Listener: validated x,y
Listener->>Calc: toRatio(validatedX,...)
Calc-->>Listener: xRatio,yRatio
Listener->>Repo: savePositionRatios(xRatio,yRatio)
Repo->>Prefs: apply() (persist ratios)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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
🧹 Nitpick comments (2)
common-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.kt (2)
15-16: Consider caching the SharedPreferences instance.The
prefscomputed property callsgetSharedPreferenceson every access. While Android internally caches the returned instance, using a lazy delegate would be slightly more efficient and explicit.♻️ Optional: Cache the SharedPreferences instance
- private val prefs: SharedPreferences - get() = context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) + private val prefs: SharedPreferences by lazy { + context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.kt` around lines 15 - 16, The prefs computed property in FabPositionRepository currently calls context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) on every access; change it to cache the SharedPreferences instance (e.g., use a lazy-initialized property) so subsequent accesses reuse the same object. Update the prefs declaration in the FabPositionRepository class to initialize once (using lazy or an explicit val assigned in the constructor/init) while still calling context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) to obtain the instance.
18-24: Minor: Naming collision between Kotlin'sapplyand SharedPreferences'apply().The code is correct, but using Kotlin's
apply {}scope function alongside the Editor'sapply()method can be confusing at first glance. Consider using chained calls or the AndroidX Core KTXedit {}extension for clarity.♻️ Optional: Clearer alternatives
Option 1 - Chained calls:
fun savePositionRatios(xRatio: Float, yRatio: Float) { - prefs.edit().apply { - putFloat(KEY_FAB_X_RATIO, xRatio) - putFloat(KEY_FAB_Y_RATIO, yRatio) - apply() - } + prefs.edit() + .putFloat(KEY_FAB_X_RATIO, xRatio) + .putFloat(KEY_FAB_Y_RATIO, yRatio) + .apply() }Option 2 - AndroidX KTX extension (if
androidx.core:core-ktxis available):fun savePositionRatios(xRatio: Float, yRatio: Float) { prefs.edit { putFloat(KEY_FAB_X_RATIO, xRatio) putFloat(KEY_FAB_Y_RATIO, yRatio) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.kt` around lines 18 - 24, The savePositionRatios function uses Kotlin's apply { } scope together with the SharedPreferences.Editor.apply() method which can be confusing; replace the block so the Editor's methods are called without nesting Kotlin's apply scope — e.g., avoid prefs.edit().apply { ... apply() } and instead use a clear pattern such as chaining (prefs.edit().putFloat(...).putFloat(...).apply()) or the AndroidX Core KTX edit { } extension (prefs.edit { putFloat(...); putFloat(...) }) to remove the naming collision and improve readability; update references in savePositionRatios and any similar methods that call prefs.edit().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common-ui/src/main/java/com/itsaky/androidide/DraggableTouchListener.kt`:
- Around line 48-53: The touch handler's when-branch currently treats canceled
gestures as else -> false and doesn't reset state; add an explicit
MotionEvent.ACTION_CANCEL case in the same when where ACTION_DOWN/MOVE/UP are
handled and call a new or existing cleanup routine (e.g., implement
handleActionCancel(fab) or reuse handleActionUp semantics) to set isDragging =
false and isLongPressed = false and perform any visual/state cleanup, then
return false; reference the when block handling MotionEvent.ACTION_DOWN,
ACTION_MOVE, ACTION_UP and the isDragging/isLongPressed flags so the cancel
logic is colocated with the other action handlers.
In `@common-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.kt`:
- Around line 57-65: The fallback default coordinates for the FAB (computed as
defaultX = marginStart and defaultY = parentView.height - fabView.height -
marginBottom in FabPositionCalculator) can still fall outside the allowed
safeBounds (e.g., very small parent height); clamp defaultX and defaultY to the
safeBounds Rect before returning so the restored position is always within
bounds. Locate the block computing marginStart/marginBottom and
defaultX/defaultY and replace the raw return with clampedX =
safeBounds.coerceInX(defaultX) and clampedY = safeBounds.coerceInY(defaultY)
(use the existing safeBounds utilities or inline min/max) so the method returns
the coordinates guaranteed to lie inside safeBounds.
- Around line 23-34: The getSafeDraggingBounds in FabPositionCalculator
currently only applies the systemBarsInsets.top; update it to extract all inset
components (left, top, right, bottom) from systemBarsInsets and use them to
compute safe bounds: subtract left inset + start/marginStart and fabMarginPx
from bounds.left, subtract right inset + fab width + marginEnd from
bounds.right, and subtract bottom inset + bottomMargin + fabMarginPx from
bounds.bottom, while keeping bounds.top based on top inset + topMargin +
fabMarginPx; reference variables/methods: getSafeDraggingBounds,
systemBarsInsets, bounds, fabMarginPx, fabView, parentView and the FAB layout
params (topMargin/marginStart/bottomMargin) to ensure consistent margin handling
across all edges.
In `@common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt`:
- Around line 70-74: The layout listener is comparing raw coordinates
(right/bottom) to stored dimensions (lastWidth/lastHeight), causing false
positives when the view moves; in the parentView.addOnLayoutChangeListener
callback compute currentWidth = right - left and currentHeight = bottom - top,
compare those to lastWidth/lastHeight, update lastWidth/lastHeight from those
computed dimensions, and only call loadFabPosition() when the computed
width/height actually changed.
---
Nitpick comments:
In `@common-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.kt`:
- Around line 15-16: The prefs computed property in FabPositionRepository
currently calls context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) on
every access; change it to cache the SharedPreferences instance (e.g., use a
lazy-initialized property) so subsequent accesses reuse the same object. Update
the prefs declaration in the FabPositionRepository class to initialize once
(using lazy or an explicit val assigned in the constructor/init) while still
calling context.getSharedPreferences(FAB_PREFS, Context.MODE_PRIVATE) to obtain
the instance.
- Around line 18-24: The savePositionRatios function uses Kotlin's apply { }
scope together with the SharedPreferences.Editor.apply() method which can be
confusing; replace the block so the Editor's methods are called without nesting
Kotlin's apply scope — e.g., avoid prefs.edit().apply { ... apply() } and
instead use a clear pattern such as chaining
(prefs.edit().putFloat(...).putFloat(...).apply()) or the AndroidX Core KTX edit
{ } extension (prefs.edit { putFloat(...); putFloat(...) }) to remove the naming
collision and improve readability; update references in savePositionRatios and
any similar methods that call prefs.edit().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c09b9ad4-f5c4-4c60-b925-3d8ec6e87153
📒 Files selected for processing (4)
common-ui/src/main/java/com/itsaky/androidide/DraggableTouchListener.ktcommon-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.ktcommon-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.ktcommon-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt
common-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.kt
Outdated
Show resolved
Hide resolved
common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt
Outdated
Show resolved
Hide resolved
jatezzz
force-pushed
the
refactor/ADFA-3263-feedback-fab-manager
branch
from
c0d97a2 to
62da823
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt (1)
38-42: Serialize restore work so concurrent loads cannot reapply stale ratios.
loadFabPosition()is hit from more than one path, and each call does an async read followed by a posted apply. That makes it possible for an older restore to land after a newer drag/save and snap the FAB back to stale coordinates. Consider either making this hot-path read synchronous or dropping stale restores with a generation/job check. Please verify by resizing or resuming the activity and dragging immediately afterward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt` around lines 38 - 42, loadFabPosition() performs an async read (repository.readPositionRatios()) and then posts applySavedPosition(fab, xRatio, yRatio), allowing older restores to overwrite newer positions; fix by serializing/resting stale restores: either make the read synchronous on the UI/thread or add a generation/token check or single-job guard in FeedbackButtonManager — increment a private generation counter or set a single Coroutine Job before launching from activity.lifecycleScope.launch, capture the current token/job in the launched coroutine, and before calling fab.post { applySavedPosition(...) } verify the token/job still matches (or that the previous job completed); apply this check around repository.readPositionRatios() and applySavedPosition(fab,...) to drop stale restores when a newer load/save/drag occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.kt`:
- Around line 22-25: The RTL bug: in FabPositionCalculator resolve logical
margins (marginStart/marginEnd) to physical left/right before creating the
bounds Rect by checking layout direction with
ViewCompat.getLayoutDirection(view); if RTL swap them (physicalLeft = marginEnd,
physicalRight = marginStart) otherwise physicalLeft = marginStart, physicalRight
= marginEnd, then use physicalLeft/physicalRight (and marginTop/marginBottom as
before) when building the Rect; apply the same change to the other occurrence
around lines 37–41 so both Rect constructions use resolved physical margins.
In `@common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt`:
- Around line 68-75: The layout-change listener only reacts to size changes, but
FabPositionCalculator.getSafeDraggingBounds() also depends on system window
insets, so add a window-insets change listener on the same parent view (e.g.,
via setOnApplyWindowInsetsListener or ViewCompat.setOnApplyWindowInsetsListener)
and call loadFabPosition() from that listener as well; ensure you keep the
existing parentView.addOnLayoutChangeListener and reference loadFabPosition() so
the FAB is repositioned when insets (navigation/taskbar) change.
---
Nitpick comments:
In `@common-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt`:
- Around line 38-42: loadFabPosition() performs an async read
(repository.readPositionRatios()) and then posts applySavedPosition(fab, xRatio,
yRatio), allowing older restores to overwrite newer positions; fix by
serializing/resting stale restores: either make the read synchronous on the
UI/thread or add a generation/token check or single-job guard in
FeedbackButtonManager — increment a private generation counter or set a single
Coroutine Job before launching from activity.lifecycleScope.launch, capture the
current token/job in the launched coroutine, and before calling fab.post {
applySavedPosition(...) } verify the token/job still matches (or that the
previous job completed); apply this check around repository.readPositionRatios()
and applySavedPosition(fab,...) to drop stale restores when a newer
load/save/drag occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d66bbd1f-a5bb-47a6-9613-4d3223a22e63
📒 Files selected for processing (4)
common-ui/src/main/java/com/itsaky/androidide/DraggableTouchListener.ktcommon-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.ktcommon-ui/src/main/java/com/itsaky/androidide/FabPositionRepository.ktcommon-ui/src/main/java/com/itsaky/androidide/FeedbackButtonManager.kt
common-ui/src/main/java/com/itsaky/androidide/FabPositionCalculator.kt
Outdated
Show resolved
Hide resolved
jatezzz
requested review from
Daniel-ADFA,
dara-abijo-adfa,
itsaky-adfa and
jomen-adfa
dara-abijo-adfa
approved these changes
Mar 19, 2026
Split FeedbackButtonManager into separate calculator, repository, and touch listener components.
Handle touch cancellation, respect all system bar insets, and reload only on real size changes
jatezzz
force-pushed
the
refactor/ADFA-3263-feedback-fab-manager
branch
from
8cf674d to
5e1fc60
Compare
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
2 participants
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.
Description
Refactored
FeedbackButtonManagerto adhere to the Single Responsibility Principle (SRP). The "God Class" was split into smaller, modular components:FabPositionCalculator: Handles safe dragging bounds and ratio math.FabPositionRepository: Manages shared preferences storage.DraggableTouchListener: Isolates the complex touch and gesture logic.Additionally, updated the FAB to use normalized position ratios instead of absolute screen coordinates. This ensures the button maintains its relative position during layout changes, multi-window mode, or screen resizing (e.g., Samsung DeX).
Details
Before fix
document_5028666762355280055.mp4
After fix
document_5028666762355280054.mp4
Ticket
ADFA-3263
Observation
All extracted support classes (
FabPositionCalculator,FabPositionRepository,DraggableTouchListener) were created withinternalvisibility to keep the module's public API surface clean.