moody / Dejunk

Simple junk selling and destroying for WoW.
https://www.curseforge.com/wow/addons/dejunk
MIT License
10 stars 3 forks source link

Disabling "Exclude unbound equipment" does not include unbound equipment. #223

Open Vettic opened 2 months ago

Vettic commented 2 months ago

I noticed this a while back and then today again while trying to sell a ton of green gear, un-checking "exclude unbound equipment" doesn't actually add that equipment to the dejunk window. If you have a bunch of BoEs to sell, dejunk does nothing.

I made a small change to the junk-filter.lua file and unbound equipment is added, including BoEs. I dont know how to do a pull request so I'll just add it here. So in your junk-filter.lua you've got exclude unbound equipment at line 147 to line 150, but if you cut and paste it down after line 167 and use this code, the system still works the same and BoEs will be included in the dejunk window when the exclude checkbox is disabled.

-- Exclude unbound equipment. if currentState.excludeUnboundEquipment and (Items:IsItemEquipment(item) and not Items:IsItemBound(item)) then return false, concat(L.OPTIONS_TEXT, L.EXCLUDE_UNBOUND_EQUIPMENT_TEXT) else if not currentState.excludeUnboundEquipment and (Items:IsItemEquipment(item) and not Items:IsItemBound(item)) then return true, concat(L.OPTIONS_TEXT, L.EXCLUDE_UNBOUND_EQUIPMENT_TEXT) end end

image

jsauerland commented 2 months ago

@Vettic @moody I was coming here to ask a similar question regarding how to include BOE gear now. I feel that it doesn't really make sense logically for the user to 'exclude unbound equipment'. Personally, it makes more sense to reword it: "Include unbound equipment/BOEs". This would make a lot more sense, as the user can simply check a box on their characters to include the BOE gear

Another issue, however, is warbound gear has changed things significantly. I'm unsure if it will affect the addon per se, but BOE gear is 'bound' to your account if you use it for transmog, so it's untradeable/unsellable on the AH, but it's "warbound" now across characters. This may need some additional thought, who knows.

Vettic commented 2 months ago

I feel that it doesn't really make sense logically for the user to 'exclude unbound equipment'. Personally, it makes more sense to reword it: "Include unbound equipment/BOEs". This would make a lot more sense, as the user can simply check a box on their characters to include the BOE gear

Yeah it is a bit of a double negative currently; "no, dont include unbound equipment" "yes, dont include unbound equipment".

It would have required more changes to code to move the option and change the text, it might interfere more with other changes. The way i added it was to be as small a change as possible.

moody commented 2 months ago

To be honest, I added this option mostly as a reaction to some retail patch where grey equipment got turned into BoEs for transmog collecting. Didn't put much thought into it outside of that.

If I were to add blanket include options for higher qualities (e.g., Include Uncommon Items, Include Epic Items, etc.) then this exclude option might make more sense.

moody commented 3 weeks ago

@Vettic @jsauerland In 2.0.0, Include Below Item Level applies to all equipment regardless of bound status. Also, item quality check boxes were added to give certain options more control.

Now, you can effectively include equipment of selected qualities (below an item level), while excluding any unbound/warbound equipment of selected qualities. Obviously, it's not the same as having an Include Unbound Equipment option, but is it close enough?

Vettic commented 3 weeks ago

Wouldn't low ilvl now also sell off BoEs? Even if you have excluded unbound equipment? That's the kind if item that players might want to keep.

Like dejunk as a tool is particularly useful when farming old raids, and that's the place you definitely want to keep BoEs from, to put on the AH.

moody commented 3 weeks ago

Wouldn't low ilvl now also sell off BoEs?

Yes, but not when Exclude Unbound Equipment is enabled. Exclude options have priority over Include options.

Vettic commented 3 weeks ago

Seems pretty complex compared to my 4 line addition, but it's your work. I can't say I need that level of granularity.

I think the difference between exclude and include is more just down to the wording and logic. "exclude" presents something of a double negative: yes do, not put unbound items in the list, no do not, not put unbound items in the list

It makes it difficult to understand, which can lead to user error, not great when you're deleting items, especially if you have safe mode turned off. Why make things difficult?

Logically it would be simpler to have: yes do, put items in the list no do not, put items in the list.

I don't really get the logic of exclusions tbh. Like who would want to include their equipment set? why is that an option that can be turned off? It would be simpler to have both of these options reversed and under the include section, no exclusion options other than user generated ones. image

moody commented 3 weeks ago

Disabling an Exclude option does not directly mark the targeted items as junk; instead, it allows these items to be potentially marked as junk by an Include option.

Enabling an Exclude option serves as a safeguard to prevent Include options from marking certain items as junk.

Vettic commented 3 weeks ago

I guess that's reason for this thread. If I disable "exclude unbound" I was expecting unbound items to be automatically added to the junk list.