Conversation
When "loot every item" is off, it will loot everything in the loot list. When "loot every item" is on, it will loot everything that is not in the loot list, effectively making the loot list a blacklist when "loot every item" is on. Related upstream issue: Vithrax/vBot#15 I have tried submitting this upstream too, but it seems vBot upstream is unmaintained: Vithrax/vBot#18
There was a problem hiding this comment.
Pull request overview
This PR adds blacklist functionality to the vBot looting system. When the "Loot every item" toggle is enabled, it now functions as a blacklist mode - looting all items except those in the list, rather than the previous behavior of simply looting everything. This enables users to filter out unwanted items while automatically looting rare items they may not know exist.
Changes:
- Modified the looting logic to treat the item list as a blacklist when "Loot every item" is enabled
- Added dynamic label that changes between "Items to loot" (whitelist mode) and "Items to ignore" (blacklist mode)
- Updated button text to indicate the blacklist functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| modules/game_bot/default_configs/vBot_4.8/targetbot/looting.otui | Added dynamic label for list purpose, updated button text to reflect blacklist mode, reduced font size to fit longer text |
| modules/game_bot/default_configs/vBot_4.8/targetbot/looting.lua | Implemented blacklist logic, added label text updates in setup and update functions to reflect current mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ui.labelToLoot then | ||
| if ui.everyItem:isOn() then | ||
| ui.labelToLoot:setText("Items to ignore") | ||
| else | ||
| ui.labelToLoot:setText("Items to loot") | ||
| end | ||
| end |
There was a problem hiding this comment.
The logic for updating the label text based on the everyItem toggle state is duplicated between the setup function (lines 17-23) and the update function (lines 62-68). Consider extracting this into a separate helper function like updateLabelText() to improve maintainability and reduce code duplication.
| !text: tr("Loot every item, except these") | ||
| margin-top: 2 | ||
| font: small-9px | ||
|
|
There was a problem hiding this comment.
The font size has been reduced to small-9px to accommodate the longer button text. This smaller font size may impact readability and accessibility, particularly for users with visual impairments. Consider if the text can be made more concise to avoid needing a smaller font, or if the UI layout can be adjusted to accommodate the standard font size.
| !text: tr("Loot every item, except these") | |
| margin-top: 2 | |
| font: small-9px | |
| !text: tr("Loot all except listed") | |
| margin-top: 2 |
| if item:isContainer() and not itemsById[item:getId()] then | ||
| nextContainer = item | ||
| elseif itemsById[item:getId()] or (ui.everyItem:isOn() and not item:isContainer()) then | ||
| elseif (not ui.everyItem:isOn() and itemsById[item:getId()]) or (ui.everyItem:isOn() and (not item:isContainer() and not itemsById[item:getId()])) then |
There was a problem hiding this comment.
The boolean logic on this line is complex and difficult to understand at a glance. Consider extracting this into a helper function with a descriptive name like shouldLootItem(item) or adding a comment explaining the two modes: whitelist mode (everyItem OFF - loot only items in list) and blacklist mode (everyItem ON - loot everything except items in list and containers).
modules/game_bot/default_configs/vBot_4.8/targetbot/looting.otui
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ui.labelToLoot then | ||
| if ui.everyItem:isOn() then | ||
| ui.labelToLoot:setText("Items to ignore") | ||
| else | ||
| ui.labelToLoot:setText("Items to loot") | ||
| end | ||
| end |
There was a problem hiding this comment.
The label update logic is duplicated in the onClick handler and the update function. Consider extracting this into a separate function to improve maintainability and reduce code duplication. For example, create a function like updateLootLabel() that checks the state and updates the label text accordingly.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| nextContainer = item | ||
| elseif itemsById[item:getId()] or (ui.everyItem:isOn() and not item:isContainer()) then | ||
| elseif (not ui.everyItem:isOn() and itemsById[item:getId()]) or (ui.everyItem:isOn() and not item:isContainer() and not itemsById[item:getId()]) then | ||
| item.lootTries = (item.lootTries or 0) + 1 |
There was a problem hiding this comment.
The looting condition is correct but hard to read/maintain as a single boolean expression with repeated lookups. Consider computing local flags (e.g., blacklistMode, isContainer, inList) and using a small if/else to decide whether to loot, so future changes don’t accidentally invert the logic.
When "loot every item" is off, it will loot everything in the loot list.
When "loot every item" is on, it will loot everything that is not in the loot list, effectively making the loot list a blacklist when "loot every item" is on.
This allows you to loot all the super-rare items you don't have the id for, or don't even know exist.
Related upstream issue: Vithrax/vBot#15
I have tried submitting this upstream too, but it seems vBot upstream is unmaintained: Vithrax/vBot#18