Port clothing layer drag-and-drop refinements#329
Open
gwigz wants to merge 1 commit into
Open
Conversation
Brings the COF clothing layer reorder feature up to the final upstream state (secondlife/viewer#5918 + secondlife/viewer#5959), on top of the two commits already cherry-picked here: - LLScrollContainer: replace the bespoke preserveScrollbarMetrics helper with a keep_scroll_pos param, so the clothing list no longer jumps its scroll position on rebuild - LLFlatListView: arm reorder drag in inter-row gaps (handleMouseDown + getReorderPairAt), cancel an active drag on visibility change (alt-tab), replace the mProcessingRightClick hack with EMouseClickType, add a reorder-ended callback fired on cancelled drags, report the moved row's visible index, plus the secondlife/viewer#5959 nullptr/constexpr/formatting cleanup - LLCOFWearables: defer a refresh that arrives mid-drag and re-run it when the drag ends, so a COF change can't cancel an active reorder - panel_cof_wearables.xml: keep_scroll_pos="true" on the clothing list Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughLLFlatListView now distinguishes left and right clicks, maps padding presses to reorder rows, and reports reorder completion. LLScrollContainer can preserve scroll position, and LLCOFWearables defers refreshes during active clothing reorders. ChangesClothing list interaction updates
Sequence Diagram(s)sequenceDiagram
participant User
participant LLFlatListView
participant LLCOFWearables
User->>LLFlatListView: drag clothing row
LLCOFWearables->>LLFlatListView: isReorderActive()
LLFlatListView-->>LLCOFWearables: true
LLCOFWearables->>LLCOFWearables: set mRefreshPending
User->>LLFlatListView: finish drag
LLFlatListView->>LLCOFWearables: onReorderEnded()
LLCOFWearables->>LLCOFWearables: refresh()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
indra/llui/llflatlistview.cpp (1)
965-983: 🩺 Stability & Availability | 🔴 CriticalCritical Double-Free in
removeItemPairdue to Callback Re-entrancy
clearReorderDragState()invokesmReorderEndedCallback()beforeremoveItemPair()has erased or deleted the target item. InLLCOFWearables, this callback triggersrefresh()->clear(), which iteratesmItemPairsand callsremoveItemPair()for every item, including the one currently being processed.Consequence:
clear()callsremoveItemPair()recursively for the active item.- The recursive call erases the item from
mItemPairsand executesdelete item_pair.- Control returns to the original
removeItemPair(), which then executesdelete item_pairagain on the same memory address.
This results in a guaranteed crash (double-free) or heap corruption.Fix Required:
MovemReorderEndedCallback()invocation to afterremoveItemPair()returns, or guardrefresh()/clear()to prevent list mutation while a removal operation is active.[Lines 1567-1590, 974-980, 434-470]
🤖 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 `@indra/llui/llflatlistview.cpp` around lines 965 - 983, The reorder-end callback is being fired from clearReorderDragState() before the active item removal finishes, which lets LLCOFWearables::refresh() re-enter clear()/removeItemPair() and double-delete the same item. Update LLFlatListView::removeItemPair() / clearReorderDragState() so mReorderEndedCallback runs only after the removal path has fully completed, or add a re-entrancy guard around clear()/refresh() while a removal is in progress. Use the symbols clearReorderDragState(), removeItemPair(), and mReorderEndedCallback to keep the fix localized.
🤖 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 `@indra/llui/llscrollcontainer.cpp`:
- Around line 698-708: The restored document positions are only being applied to
the scrollbar state in the mKeepScrollPos branch, so mScrolledView can still
render at the old or clamped location until the next updateScroll() pass. Update
the LLScrollContainer flow so the restored positions are applied immediately to
the scrolled view after scrollVertical() and scrollHorizontal() run, using the
existing mStoredDocPos values and the related scrollbar/doc-position logic in
LLScrollContainer rather than waiting for a later refresh.
---
Outside diff comments:
In `@indra/llui/llflatlistview.cpp`:
- Around line 965-983: The reorder-end callback is being fired from
clearReorderDragState() before the active item removal finishes, which lets
LLCOFWearables::refresh() re-enter clear()/removeItemPair() and double-delete
the same item. Update LLFlatListView::removeItemPair() / clearReorderDragState()
so mReorderEndedCallback runs only after the removal path has fully completed,
or add a re-entrancy guard around clear()/refresh() while a removal is in
progress. Use the symbols clearReorderDragState(), removeItemPair(), and
mReorderEndedCallback to keep the fix localized.
🪄 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: cdaed76b-fdaf-4c45-98f2-6e461d9aa829
📒 Files selected for processing (8)
indra/llui/llflatlistview.cppindra/llui/llflatlistview.hindra/llui/llscrollcontainer.cppindra/llui/llscrollcontainer.hindra/newview/llappearancemgr.hindra/newview/llcofwearables.cppindra/newview/llcofwearables.hindra/newview/skins/default/xui/en/panel_cof_wearables.xml
Contributor
Author
|
@coderabbitai resolve
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="AlchemyViewer/Alchemy"
pr_number="329"
echo "== Review comments by coderabbitai on PR #$pr_number =="
gh api "repos/$repo/pulls/$pr_number/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login=="coderabbitai") | {id, node_id, path, line, body, url}'
echo
echo "== Matching comments mentioning removeItemPair or clear() =="
gh api "repos/$repo/pulls/$pr_number/comments?per_page=100" --paginate \
--jq '.[] | select(.user.login=="coderabbitai") | select((.body | test("removeItemPair|clear\\(\\)"; "i"))) | {id, node_id, path, line, body, url}'
echo
echo "== Check relevant file outline =="
fd -i 'llflatlistview.cpp' . -x sh -c 'echo "--- $1"; ast-grep outline "$1" --view expanded | sed -n "1,220p"' sh {}
echo
echo "== Search for clear() and removeItemPair() definitions/usages in LLFlatListView =="
fd -i 'llflatlistview.cpp' . -x sh -c '
file="$1"
echo "--- $file"
rg -n -C3 "\\b(clear|removeItemPair)\\s*\\(" "$file"
' sh {}Length of output: 5037 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="AlchemyViewer/Alchemy"
owner="${repo%%/*}"
name="${repo#*/}"
pr_number=329
echo "== PR review threads with comments by coderabbitai =="
gh api graphql \
-F owner="$owner" \
-F name="$name" \
-F pr="$pr_number" \
-f query='
query($owner: String!, $name: String!, $pr: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $pr) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 20) {
nodes {
id
databaseId
url
body
author { login }
}
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login=="coderabbitai"))
| {
thread_id: .id,
isResolved,
isOutdated,
path,
line,
comments: [.comments.nodes[] | {author: .author.login, databaseId, url, body}]
}'
echo
echo "== Top-level PR issue comments mentioning removeItemPair or clear() =="
gh api "repos/$repo/issues/$pr_number/comments?per_page=100" --paginate \
--jq '.[] | select((.body | test("removeItemPair|clear\\(\\)|resolve"; "i"))) | {id, user: .user.login, body, url}'Length of output: 7021
✏️ Learnings added
|
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.
Brings the COF clothing layer reorder feature up to upstream state: