feat(inventory-list): add smooth highlighting to items in the inventory list#181
feat(inventory-list): add smooth highlighting to items in the inventory list#181zndxcvbn wants to merge 7 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis pull request introduces selection animation control to SkyUI's list components. ScrollingList gains animation-disable flags and animation baseline tracking. TabularListEntry implements selection animation with fade-in and jump-decay effects via onEnterFrame callbacks. Subclass initialization methods are updated to pass index context to the parent class. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ScrollingList
participant TabularListEntry
participant AnimationSystem as Animation System<br/>(onEnterFrame)
User->>ScrollingList: Select list item
Note over ScrollingList: Check bDisableAnim flag
alt bDisableAnim is true
ScrollingList->>TabularListEntry: updateSelectionAnimation(true)
TabularListEntry->>TabularListEntry: Immediately set indicator visible<br/>Record Y to lastSelectionAnimY
else bDisableAnim is false
ScrollingList->>TabularListEntry: updateSelectionAnimation(true)
TabularListEntry->>TabularListEntry: Make indicator visible
alt No prior animation Y baseline
TabularListEntry->>AnimationSystem: Register onEnterFrame (fade-in)
AnimationSystem->>TabularListEntry: Animate opacity increase
else Prior Y baseline exists
TabularListEntry->>AnimationSystem: Register onEnterFrame (jump-decay)
AnimationSystem->>TabularListEntry: Animate Y jump and opacity<br/>based on lastSelectionAnimY
end
end
User->>ScrollingList: Deselect or move selection
ScrollingList->>TabularListEntry: updateSelectionAnimation(false)
TabularListEntry->>TabularListEntry: resetSelectionAnim()<br/>Clear animation state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 41 minutes and 41 seconds.Comment |
zndxcvbn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
source/actionscript/Common/skyui/components/list/TabularListEntry.as (2)
127-130: Heads-up:diffY == 0short-circuit leaves any in-flightonEnterFramerunning.When the same entry is re-selected at the same
_y(e.g., redundantsetEntryfrom a layout re-evaluation), an in-progress jump-decay or fade-in continues to completion. That's almost certainly the intended behavior (don't restart an animation when nothing moved), but worth confirming — if you ever want re-selection to snap-finish a decay, you'd need an explicitresetSelectionAnim(true)here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/TabularListEntry.as` around lines 127 - 130, The early return in TabularListEntry (inside the setEntry/selection handling where it checks diffY == 0) leaves any in-flight onEnterFrame animation running; to ensure re-selection can snap-finish animations when desired, call resetSelectionAnim(true) before returning (or document why you intentionally want to keep animations running). Specifically, update the block that computes prevY/ diffY in TabularListEntry to invoke resetSelectionAnim(true) when diffY == 0 if you want to immediately finish the current jump-decay/fade-in, otherwise add a comment explaining why the current behavior is correct so maintainers know this was intentional.
101-101: Tightena_listparameter typing.
a_list: Objectis permissive; the body readsbDisableAnimandlastSelectionAnimY, which are defined only onScrollingList. Typing it asskyui.components.list.ScrollingList(or asBasicListif you'd prefer to relocate those fields up the hierarchy) would catch accidental misuse at compile time and document the coupling that already exists in practice.♻️ Proposed type tightening
- public function updateSelectionAnimation(isSelected: Boolean, a_list: Object) + public function updateSelectionAnimation(isSelected: Boolean, a_list: skyui.components.list.ScrollingList)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/TabularListEntry.as` at line 101, The method updateSelectionAnimation currently accepts a_list: Object but reads properties (bDisableAnim, lastSelectionAnimY) that exist on ScrollingList; change the parameter type on updateSelectionAnimation to skyui.components.list.ScrollingList (or to BasicList if you prefer to promote bDisableAnim/lastSelectionAnimY up the class hierarchy) and update any imports/usages accordingly so the compiler enforces the tighter type and existing references inside updateSelectionAnimation remain valid.source/actionscript/Common/skyui/components/list/ScrollingList.as (2)
330-335: Consider exception safety (or document the assumption).If
updateScrollPositionthrows,bDisableAnimwould be left stuck attrueand all subsequent selections would skip animation. AS2 doesn't have try/finally as ergonomically as AS3, but given Flash usually swallows exceptions silently, the practical risk is low. Worth noting in case animation ever appears "stuck off" in the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/ScrollingList.as` around lines 330 - 335, The onScroll handler currently sets bDisableAnim = true before calling updateScrollPosition and resets it after, which can leave bDisableAnim true if updateScrollPosition throws; modify onScroll to ensure bDisableAnim is always reset (use a try/catch/finally-style pattern available in this environment or explicitly catch errors around updateScrollPosition and restore bDisableAnim in the catch and after normal execution), referencing the onScroll function, the updateScrollPosition call, and the bDisableAnim field, or alternatively add a clear comment on the assumption that updateScrollPosition cannot throw and why that is safe.
40-49: Animation guard bypassed whenscrollbaris undefined.
onScroll(lines 332–334) wrapsupdateScrollPositionwithbDisableAnim = true/false, but theelsebranch here callsupdateScrollPositiondirectly without the same guard. Whenscrollbaris undefined (theonLoadguard shows that's possible), programmatic scrolls via this setter — including those triggered indirectly bymoveSelectionUp/DownandonMouseWheel— will run UpdateList withbDisableAnim = false, so the selected entry's_ychange re-enters the jump-decay path on every scroll instead of being treated as a layout reset.♻️ Proposed fix to keep the no-scrollbar path consistent
if (this.scrollbar != undefined) this.scrollbar.position = a_newPosition; - else - this.updateScrollPosition(a_newPosition); + else { + this.bDisableAnim = true; + this.updateScrollPosition(a_newPosition); + this.bDisableAnim = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/ScrollingList.as` around lines 40 - 49, The setter scrollPosition currently calls updateScrollPosition directly when scrollbar is undefined, bypassing the animation-disable guard used by onScroll; change the else branch in the scrollPosition setter to call updateScrollPosition in the same guarded way as onScroll (i.e., pass the bDisableAnim flag or invoke the same wrapper) so programmatic scrolls (from moveSelectionUp/Down, onMouseWheel, etc.) call updateScrollPosition with bDisableAnim=true and preserve the no-animation/layout-reset semantics used by onScroll and onLoad.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/actionscript/Common/skyui/components/list/ScrollingList.as`:
- Around line 330-335: The onScroll handler currently sets bDisableAnim = true
before calling updateScrollPosition and resets it after, which can leave
bDisableAnim true if updateScrollPosition throws; modify onScroll to ensure
bDisableAnim is always reset (use a try/catch/finally-style pattern available in
this environment or explicitly catch errors around updateScrollPosition and
restore bDisableAnim in the catch and after normal execution), referencing the
onScroll function, the updateScrollPosition call, and the bDisableAnim field, or
alternatively add a clear comment on the assumption that updateScrollPosition
cannot throw and why that is safe.
- Around line 40-49: The setter scrollPosition currently calls
updateScrollPosition directly when scrollbar is undefined, bypassing the
animation-disable guard used by onScroll; change the else branch in the
scrollPosition setter to call updateScrollPosition in the same guarded way as
onScroll (i.e., pass the bDisableAnim flag or invoke the same wrapper) so
programmatic scrolls (from moveSelectionUp/Down, onMouseWheel, etc.) call
updateScrollPosition with bDisableAnim=true and preserve the
no-animation/layout-reset semantics used by onScroll and onLoad.
In `@source/actionscript/Common/skyui/components/list/TabularListEntry.as`:
- Around line 127-130: The early return in TabularListEntry (inside the
setEntry/selection handling where it checks diffY == 0) leaves any in-flight
onEnterFrame animation running; to ensure re-selection can snap-finish
animations when desired, call resetSelectionAnim(true) before returning (or
document why you intentionally want to keep animations running). Specifically,
update the block that computes prevY/ diffY in TabularListEntry to invoke
resetSelectionAnim(true) when diffY == 0 if you want to immediately finish the
current jump-decay/fade-in, otherwise add a comment explaining why the current
behavior is correct so maintainers know this was intentional.
- Line 101: The method updateSelectionAnimation currently accepts a_list: Object
but reads properties (bDisableAnim, lastSelectionAnimY) that exist on
ScrollingList; change the parameter type on updateSelectionAnimation to
skyui.components.list.ScrollingList (or to BasicList if you prefer to promote
bDisableAnim/lastSelectionAnimY up the class hierarchy) and update any
imports/usages accordingly so the compiler enforces the tighter type and
existing references inside updateSelectionAnimation remain valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 61be3f79-7248-48c2-8140-eca0ab0efcdb
📒 Files selected for processing (4)
source/actionscript/Common/skyui/components/list/ScrollingList.assource/actionscript/Common/skyui/components/list/TabularListEntry.assource/actionscript/CraftingMenu/CraftingListEntry.assource/actionscript/ItemMenus/InventoryListEntry.as
1 participant
This PR adds a smooth highlight animation when hovering over inventory items.
Tasks:
Summary by CodeRabbit
Release Notes