feat: pending highest roller and hover tooltips for loot history (#90)#154
Open
feat: pending highest roller and hover tooltips for loot history (#90)#154
Conversation
- Add pendingText fontstring to history entries showing current highest roller in their class color while a roll is in progress - Switch to class-colored winner name when roll resolves - Show "(waiting on rolls)" greyed out when all rolls are passes - Hover tooltip on history entries now appends a sorted roll breakdown (highest first, passes last) to the existing item tooltip, with class-colored player names - Click-to-expand inline behavior preserved - Add 3 enUS locale keys: "Highest: %s (%d)", "(waiting on rolls)", "Rolls:"
Xerrion
added
C-Feature
📝 WalkthroughWalkthroughThe changes extend the loot history UI to display the current highest roller during active rolls and show detailed roll results in a tooltip on entry hover. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 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. 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.
🧹 Nitpick comments (2)
DragonLoot/Display/HistoryFrame.lua (2)
678-686: Comment is slightly misleading — branch also covers Classic's pre-assignment state.The
elseif data.rollResults and#data.rollResults> 0branch fires not only when "all passes" but also when non-pass rolls exist without numeric values yet (per the ClassicLOOT_HISTORY_ROLL_CHANGEDquirk noted in repo learnings). The "(waiting on rolls)" label is actually the correct UX for both, so behavior is fine — just the inline comment understates the cases.📝 Suggested comment tweak
elseif data.rollResults and `#data.rollResults` > 0 then - -- All passes - rare but possible + -- Either all passes, or non-pass rolls without numeric values yet + -- (Classic LOOT_HISTORY_ROLL_CHANGED can fire before roll is assigned) entry.pendingText:SetText(L["(waiting on rolls)"])Based on learnings: "Classic LOOT_HISTORY_ROLL_CHANGED may fire before roll value is assigned - skip non-Pass rolls with nil values and rely on a later re-fire with the value".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/HistoryFrame.lua` around lines 678 - 686, The inline comment above the elseif that checks data.rollResults and `#data.rollResults` > 0 is misleading because that branch also covers Classic's pre-assignment state where non-pass rolls exist but their numeric values are still nil (see LOOT_HISTORY_ROLL_CHANGED quirk); update the comment near that conditional (referencing data.rollResults and entry.pendingText:SetText(L["(waiting on rolls)"])) to note it handles both "all passes" and Classic pre-assignment non-numeric rolls (and that we intentionally show "(waiting on rolls)"), and optionally mention the behavior to skip/await rolls with nil values until a later update.
659-671: Leader-computation could share the pass/sort logic fromOnEntryEnter.Both
OnEntryEnter(lines 491–498) and this block decide what counts as a "pass" (rollType == 0or missingroll) and pick a max. Today they're consistent, but they're easy to drift if someone tweaks one and forgets the other. A tinylocal function GetHighestNonPass(rollResults) ... endwould let both paths share truth. Optional, purely a maintainability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/HistoryFrame.lua` around lines 659 - 671, Extract a small shared function (e.g., local function GetHighestNonPass(rollResults)) that encapsulates the current logic for skipping passes (rollType == 0 or missing roll) and returning the highest roll tuple (playerName, playerClass, roll); then replace the duplicate leader-computation in the HistoryFrame.lua block (the loop that sets leaderName/leaderClass/leaderRoll) and the logic inside OnEntryEnter to call GetHighestNonPass(rollResults) instead. Ensure the new function accepts the rollResults table, applies the same filtering (ignore nil roll or rollType == 0), compares numeric roll values to pick the max, and returns nils if none found so both call sites keep their existing variable assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DragonLoot/Display/HistoryFrame.lua`:
- Around line 678-686: The inline comment above the elseif that checks
data.rollResults and `#data.rollResults` > 0 is misleading because that branch
also covers Classic's pre-assignment state where non-pass rolls exist but their
numeric values are still nil (see LOOT_HISTORY_ROLL_CHANGED quirk); update the
comment near that conditional (referencing data.rollResults and
entry.pendingText:SetText(L["(waiting on rolls)"])) to note it handles both "all
passes" and Classic pre-assignment non-numeric rolls (and that we intentionally
show "(waiting on rolls)"), and optionally mention the behavior to skip/await
rolls with nil values until a later update.
- Around line 659-671: Extract a small shared function (e.g., local function
GetHighestNonPass(rollResults)) that encapsulates the current logic for skipping
passes (rollType == 0 or missing roll) and returning the highest roll tuple
(playerName, playerClass, roll); then replace the duplicate leader-computation
in the HistoryFrame.lua block (the loop that sets
leaderName/leaderClass/leaderRoll) and the logic inside OnEntryEnter to call
GetHighestNonPass(rollResults) instead. Ensure the new function accepts the
rollResults table, applies the same filtering (ignore nil roll or rollType ==
0), compares numeric roll values to pick the max, and returns nils if none found
so both call sites keep their existing variable assignments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 54dcd949-9342-47d7-825b-dc1658a71be1
📒 Files selected for processing (2)
DragonLoot/Display/HistoryFrame.luaDragonLoot/Locales/enUS.lua
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.
Description
Closes #90
Adds three improvements to the loot history frame:
Pending highest roller: While a roll is in progress, the entry shows the current highest roller in their class color, formatted
Highest: PlayerName (87). When the roll resolves, this is replaced by the class-colored winner name as before. If all current rolls are passes, the entry shows(waiting on rolls)in grey.Hover tooltip with roll details: Hovering a history entry now shows the standard item tooltip (unchanged) plus an appended
Rolls:section listing every roll, sorted highest non-pass first, then passes at the bottom. Each row shows the player name in their class color and the roll value with type (87 (Need)) on the right.Item tooltip on icon hover: The existing entry-wide hover already covers the icon area. No separate code path was needed; the same combined tooltip is shown whether the cursor is over the icon or anywhere else on the row.
Click-to-expand inline detail rows (governed by
db.profile.history.showRollDetails) is preserved unchanged - both interactions now coexist.Type of change
How has this been tested?
luacheck .passes with 0 new warnings (31 pre-existing inLibs/DragonWidgets/).Checklist
Affected versions
Summary by CodeRabbit