Conversation
…crafting menus
Implements momentum-based smooth scrolling across the SkyUI item-menu family
(inventory, container, barter, gift, magic, crafting). Wheel ticks add velocity
to a list-level tweener; subsequent frames advance the visual scroll position
with friction-based decay. Two cadence modes:
- Curve (default): chain-count tracking. Each rapid wheel tick within
ACCEL_WINDOW_MS extends a chain; per-tick impulse multiplier grows along
a quadratic curve up to CURVE_ACCEL_MAX_MULT. Spam-flicking the wheel
"winds up" -- single tick = ~1 row, four rapid ticks = ~12 rows.
- Classic: gap-based per-tick multiplier (same shape as the original draft).
Each rapid tick scales by gap-since-last; doesn't compound across the chain.
MCM exposes Smooth Scrolling (toggle) and Scroll Duration (50-500 ms) under
the existing ItemList page.
Architecture:
- ScrollingList.as: wheel handler, tween driver, mask + entriesContainer
plumbing for sub-row visual offset.
- ScrollTweener.as (new): velocity math, chain-count cadence, mode selector.
- TabularList.as: registers configLoad / configUpdate listeners and reads
the two MCM keys (enabled, durationMs) onto inherited fields. Inner-list
instances (ItemList / CraftingItemList stage classes) inherit from
TabularList, so they pick up the handling automatically -- no controller-
level forwarder needed in InventoryLists / CraftingLists.
- BasicListEntry.as: hit-test handlers walk up the display tree to find
the owning list (entriesContainer nesting can put _parent on the
container, not the list).
- EntryClipManager.as: routes new entry clips into entriesContainer when
the list provides one; falls back to the list itself otherwise.
- swfsources.cmake: TabularList.as added to the components/list injection
block for both craftingmenu and skyui_inventorylists, so the patches
actually land in the deployed bytecode.
📝 WalkthroughWalkthroughAdds a velocity-based ScrollTweener and integrates momentum scrolling into ScrollingList, updates entry delegation/attachment, exposes runtime MCM controls and migration, and updates SWF build/exports. ChangesSmooth Momentum Scrolling System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
source/actionscript/Common/skyui/components/list/ScrollTweener.as (2)
172-182: 💤 Low value
settle()andcancel()are identical — collapse or differentiate.Both methods zero velocity and deactivate. Callers already use them with intentional semantics ("settle = natural end" vs "cancel = forced abort"), but the bodies are the same. Either:
- Delegate one to the other for clarity, or
- Drop one if the semantic distinction isn't needed.
♻️ Option: delegate cancel → settle
public function cancel() { - this._velocity = 0; - this._active = false; + this.settle(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/actionscript/Common/skyui/components/list/ScrollTweener.as` around lines 172 - 182, The methods settle() and cancel() are identical; to preserve their semantic intent while avoiding duplication, have cancel() delegate to settle() instead of duplicating logic: replace cancel()'s body with a single call to settle() so settle() remains the single implementation that sets _velocity = 0 and _active = false; update any comments if needed to reflect that cancel simply invokes settle.
49-51: 💤 Low valueUse the named constant for the default cadence mode.
Initializing
CADENCE_MODEwith the literal0works (because of declaration order constraints) but is fragile ifCADENCE_MODE_CURVE's value ever changes. AS2 supports this in a static initializer block or, more simply, leaveCADENCE_MODEuninitialized at declaration and assign in astatic {}block — or add a comment cross-referencing the chosen mode.♻️ Suggested clarification
private static var CADENCE_MODE_CURVE: Number = 0; private static var CADENCE_MODE_CLASSIC: Number = 1; - private static var CADENCE_MODE: Number = 0; + private static var CADENCE_MODE: Number = 0; // = CADENCE_MODE_CURVE🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/actionscript/Common/skyui/components/list/ScrollTweener.as` around lines 49 - 51, The CADENCE_MODE is currently initialized with the literal 0 which is fragile; change it to use the named constant by removing the literal initialization and set CADENCE_MODE = CADENCE_MODE_CURVE in a static initializer (or leave uninitialized at declaration and assign it inside a static { } block) so CADENCE_MODE references CADENCE_MODE_CURVE directly; update references to CADENCE_MODE as needed and/or add a short comment explaining the default maps to CADENCE_MODE_CURVE.source/actionscript/Common/skyui/components/list/ScrollingList.as (1)
581-588: 💤 Low valueBounds-check change is correct given the new
clipCountpolicy.Allowing
a_index == _maxListIndexhere is necessary now thatUpdateListrequests_maxListIndex + 1clips during a fractional glide.EntryClipManager.getClipstill gates ona_index >= _clipCount, so callers that ask for an index beyond the currently-sized pool (i.e. whenfractional == 0) still getundefinedsafely. Worth a brief inline note that the relaxed upper bound is paired with the dynamicclipCountinUpdateListso future readers don't tighten it again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/actionscript/Common/skyui/components/list/ScrollingList.as` around lines 581 - 588, Relax the bounds check in getClipByIndex so it permits a_index == _maxListIndex (to match UpdateList requesting _maxListIndex + 1 clips during fractional glides) and add a brief inline comment explaining this pairing with the dynamic clipCount; reference getClipByIndex, UpdateList, _maxListIndex, clipCount and EntryClipManager.getClip to note that EntryClipManager.getClip still guards against a_index >= _clipCount so callers remain safe when fractional == 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/actionscript/Common/skyui/components/list/ScrollingList.as`:
- Around line 442-446: The momentum scroller starts a process-wide setInterval
in onMouseWheel which can keep firing after the list MovieClip is removed; add
an onUnload method on the ScrollingList class that checks and clears the
interval (_tickIntervalId) and cancels the momentum state (_isMomentumActive)
and any active tween (the same cleanup done when tickScrollTween settles or
scrollPosition setter runs). In onUnload call
clearInterval(this._tickIntervalId) if set, set this._tickIntervalId = null and
this._isMomentumActive = false, and ensure any tween-related state/method (the
one tickScrollTween relies on) is stopped/cleared to prevent callbacks on a
destroyed object. Ensure onUnload is invoked by the lifecycle or parent so the
interval is always cleared when the list is removed.
In `@source/actionscript/Common/skyui/components/list/TabularList.as`:
- Around line 37-46: The TabularList constructor is re-registering ConfigManager
callbacks already registered by the parent ScrollingList, causing
onConfigLoad/onConfigUpdate to run twice; remove the duplicate
skyui.util.ConfigManager.registerLoadCallback(this, "onConfigLoad") and
skyui.util.ConfigManager.registerUpdateCallback(this, "onConfigUpdate") calls
from TabularList's constructor (after super()) and rely on the parent's
registration so the overridden TabularList.onConfigLoad and onConfigUpdate are
invoked once via prototype dispatch.
---
Nitpick comments:
In `@source/actionscript/Common/skyui/components/list/ScrollingList.as`:
- Around line 581-588: Relax the bounds check in getClipByIndex so it permits
a_index == _maxListIndex (to match UpdateList requesting _maxListIndex + 1 clips
during fractional glides) and add a brief inline comment explaining this pairing
with the dynamic clipCount; reference getClipByIndex, UpdateList, _maxListIndex,
clipCount and EntryClipManager.getClip to note that EntryClipManager.getClip
still guards against a_index >= _clipCount so callers remain safe when
fractional == 0.
In `@source/actionscript/Common/skyui/components/list/ScrollTweener.as`:
- Around line 172-182: The methods settle() and cancel() are identical; to
preserve their semantic intent while avoiding duplication, have cancel()
delegate to settle() instead of duplicating logic: replace cancel()'s body with
a single call to settle() so settle() remains the single implementation that
sets _velocity = 0 and _active = false; update any comments if needed to reflect
that cancel simply invokes settle.
- Around line 49-51: The CADENCE_MODE is currently initialized with the literal
0 which is fragile; change it to use the named constant by removing the literal
initialization and set CADENCE_MODE = CADENCE_MODE_CURVE in a static initializer
(or leave uninitialized at declaration and assign it inside a static { } block)
so CADENCE_MODE references CADENCE_MODE_CURVE directly; update references to
CADENCE_MODE as needed and/or add a short comment explaining the default maps to
CADENCE_MODE_CURVE.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70057cb1-db99-40fe-8b88-4a6d13674b11
📒 Files selected for processing (10)
data/interface/translations/skyui_se_english.txtsource/actionscript/Common/skyui/components/list/BasicListEntry.assource/actionscript/Common/skyui/components/list/EntryClipManager.assource/actionscript/Common/skyui/components/list/ScrollTweener.assource/actionscript/Common/skyui/components/list/ScrollingList.assource/actionscript/Common/skyui/components/list/TabularList.assource/scripts/SKI_ConfigMenu.pscsource/swf/craftingmenu.xmlsource/swf/skyui/inventorylists.xmlsource/swfsources.cmake
…ttle/cancel ScrollingList config listener removed - was moved to TabularList. onUnload clears setInterval if the menu tears down mid-glide. settle() dropped, was same as cancel().
Cadence mode was planned, but dropped. Removed CLASSIC_FRICTION_PER_FRAME, CLASSIC_ACCEL_MAX_MULT, CADENCE_MODE_* + mode-branching in impulse()/tick(). CURVE_* -> bare ACCEL_MAX_MULT etc. Cleaned design comments from ScrollingList, ScrollTweener, TabularList, BasicListEntry, EntryClipManager.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/actionscript/Common/skyui/components/list/ScrollingList.as`:
- Around line 409-419: The setter that forwards to scrollbar.position must
cancel any active momentum so programmatic writes aren't ignored: in the
scrollPosition setter (the code that does "scrollbar.position = a_newPosition")
check and cancel the active tween/momentum (clear whatever tween/timer and set
_isMomentumActive = false) before forwarding or immediately after, then call
updateScrollPosition(newPos) (or ensure onScroll will apply it) so keyboard
paths like moveSelectionUp/moveSelectionDown and external writes take effect;
alternatively, in onScroll detect origin and if the event is from an explicit
setter, cancel the momentum and proceed (see _isMomentumActive, onScroll,
tickScrollTween, updateScrollPosition, and the scrollbar.position write).
In `@source/actionscript/Common/skyui/components/list/ScrollTweener.as`:
- Around line 97-101: The cancel() method leaves chain metadata (_chainCount and
_lastImpulseTime) intact so a subsequent impulse() within ACCEL_WINDOW_MS
inherits previous cadence; update cancel() to also reset _chainCount to 0 and
_lastImpulseTime to 0 (or a sentinel indicating "no prior impulse") so the next
impulse() starts a fresh chain and cannot immediately get ACCEL_MAX_MULT
cadence.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5557ec2b-e733-4fcb-8a47-a2706122277c
📒 Files selected for processing (5)
source/actionscript/Common/skyui/components/list/BasicListEntry.assource/actionscript/Common/skyui/components/list/EntryClipManager.assource/actionscript/Common/skyui/components/list/ScrollTweener.assource/actionscript/Common/skyui/components/list/ScrollingList.assource/actionscript/Common/skyui/components/list/TabularList.as
🚧 Files skipped from review as they are similar to previous changes (3)
- source/actionscript/Common/skyui/components/list/TabularList.as
- source/actionscript/Common/skyui/components/list/EntryClipManager.as
- source/actionscript/Common/skyui/components/list/BasicListEntry.as
| private function onScroll(event: Object) | ||
| { | ||
| this.updateScrollPosition(Math.floor(event.position + 0.5)); | ||
| // Ignore scrollbar events while momentum is driving position; otherwise the scrollbar | ||
| // event would snap us back and cancel the tween. | ||
| if (this._isMomentumActive) | ||
| return; | ||
| var newPos: Number = Math.floor(event.position + 0.5); | ||
| if (newPos == this._scrollPosition) | ||
| return; | ||
| this.updateScrollPosition(newPos); | ||
| } |
There was a problem hiding this comment.
Programmatic scrollPosition writes are silently swallowed during momentum.
The setter at lines 54-63 forwards to scrollbar.position = a_newPosition, which dispatches the "scroll" event into onScroll. With _isMomentumActive true, onScroll returns at line 414 without applying the new position, so _scrollPosition / _visualScrollPosition / UpdateList never update — only the scrollbar thumb visual moves, and even that gets stomped on the next tickScrollTween (line 449). Concretely:
moveSelectionUp/moveSelectionDownkeyboard paths (lines 321-322, 325, 348-349, 352) callingthis.scrollPosition = ...are no-ops while a wheel glide is in flight.- The same applies to any external consumer setting
scrollPositionduring a tween.
The _isMomentumActive guard was intended to suppress self-fired scroll events from line 449, but it can't distinguish them from a user/programmatic write. Cancel the tween in the setter (or in onScroll for non-self-originated changes) so explicit position changes win.
🛠️ Proposed fix — cancel momentum in the setter
public function set scrollPosition(a_newPosition: Number)
{
if (a_newPosition == this._scrollPosition || a_newPosition < 0 || a_newPosition > this._maxScrollPosition)
return;
-
+
+ // Programmatic scrollPosition takes precedence over an in-flight tween.
+ if (this._isMomentumActive) {
+ this._scrollTweener.cancel();
+ this._isMomentumActive = false;
+ if (this._tickIntervalId != -1) {
+ clearInterval(this._tickIntervalId);
+ this._tickIntervalId = -1;
+ }
+ }
+
if (this.scrollbar != undefined)
this.scrollbar.position = a_newPosition;
else
this.updateScrollPosition(a_newPosition);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/actionscript/Common/skyui/components/list/ScrollingList.as` around
lines 409 - 419, The setter that forwards to scrollbar.position must cancel any
active momentum so programmatic writes aren't ignored: in the scrollPosition
setter (the code that does "scrollbar.position = a_newPosition") check and
cancel the active tween/momentum (clear whatever tween/timer and set
_isMomentumActive = false) before forwarding or immediately after, then call
updateScrollPosition(newPos) (or ensure onScroll will apply it) so keyboard
paths like moveSelectionUp/moveSelectionDown and external writes take effect;
alternatively, in onScroll detect origin and if the event is from an explicit
setter, cancel the momentum and proceed (see _isMomentumActive, onScroll,
tickScrollTween, updateScrollPosition, and the scrollbar.position write).
There was a problem hiding this comment.
Can we skip this please? Kind of out of scope here. What person has one hand on the arrow keys and the other on the scroll wheel 😭
There was a problem hiding this comment.
@biipbiiip Fair enough 😄 — the overlap window where someone would hit arrow keys mid-glide is vanishingly small in practice. Happy to skip it!
| public function cancel() | ||
| { | ||
| this._velocity = 0; | ||
| this._active = false; | ||
| } |
There was a problem hiding this comment.
cancel() leaves chain state stale, allowing fresh impulses to inherit accelerated cadence.
cancel() resets _velocity and _active, but leaves _chainCount and _lastImpulseTime untouched. After an edge clamp (tickScrollTween lines 429/432) or any other cancel, the next impulse() from rest will see _velocity == 0, so the reversal branch in line 49 won't fire. With sinceLast still inside ACCEL_WINDOW_MS, line 52 increments rather than resetting _chainCount, so a freshly-started scroll can immediately receive cadenceMult near ACCEL_MAX_MULT (12×) and shoot away on the first wheel tick.
🛠️ Proposed fix
public function cancel()
{
this._velocity = 0;
this._active = false;
+ this._chainCount = 0;
+ this._lastImpulseTime = 0;
}Alternatively, treat _velocity == 0 as a chain-reset condition in impulse():
- if (reversing || sinceLast <= 0 || sinceLast >= window)
+ if (reversing || this._velocity == 0 || sinceLast <= 0 || sinceLast >= window)
this._chainCount = 1;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function cancel() | |
| { | |
| this._velocity = 0; | |
| this._active = false; | |
| } | |
| public function cancel() | |
| { | |
| this._velocity = 0; | |
| this._active = false; | |
| this._chainCount = 0; | |
| this._lastImpulseTime = 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@source/actionscript/Common/skyui/components/list/ScrollTweener.as` around
lines 97 - 101, The cancel() method leaves chain metadata (_chainCount and
_lastImpulseTime) intact so a subsequent impulse() within ACCEL_WINDOW_MS
inherits previous cadence; update cancel() to also reset _chainCount to 0 and
_lastImpulseTime to 0 (or a sentinel indicating "no prior impulse") so the next
impulse() starts a fresh chain and cannot immediately get ACCEL_MAX_MULT
cadence.
There was a problem hiding this comment.
I'd like to skip this please - I don't want the chain to reset on cancel/edge clamp
There was a problem hiding this comment.
@biipbiiip, understood — preserving _chainCount and _lastImpulseTime across cancel() is intentional so that a scroll resuming after an edge clamp (or any other cancellation) continues building cadence naturally rather than snapping back to a 1× multiplier. I'll drop this suggestion.
✏️ Learnings added
Learnt from: biipbiiip
Repo: doodlum/SkyUI-Community PR: 195
File: source/actionscript/Common/skyui/components/list/ScrollTweener.as:97-101
Timestamp: 2026-05-09T17:26:49.955Z
Learning: In `source/actionscript/Common/skyui/components/list/ScrollTweener.as`, `cancel()` intentionally does NOT reset `_chainCount` or `_lastImpulseTime`. The author wants chain cadence to persist across cancellations and edge clamps so that a scroll restarting shortly after still benefits from the accumulated impulse chain rather than resetting to 1× multiplier. Do not flag this as a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Implements momentum-based smooth scrolling across the SkyUI item-menu family (inventory, container, barter, gift, magic, crafting).
MCM exposes Smooth Scrolling (toggle) and Scroll Duration (50-500 ms) under the existing ItemList page.
Architecture:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes