Baggins

Baggins

1M Downloads

Equipment set item filter does not work

Elberet opened this issue ยท 12 comments

commented

The equipment set item filter does not appear to work in Cataclysm Classic. I have several equipment sets for my character for various activities - default, fishing, cooking, RP - and those sets are listed correctly, but despite selecting any combination of sets or the "Any" option, the filter does not match any items, regardless of the equipment set or sets they belong to.

Since each change of any of the filter's checkboxes triggered #118, I did a /reload after each change to test the filter's functionality.

This was observed on Cataclysm Classic with Baggins 5.4.1 with the english language client.
I have not tested this filter with any previous version of Baggins.

commented

The equipment set filter does not work very well on retail either. I have had an issue open (Stanzilla/WoWUIBugs#351) for a long time. I honestly don't know if it is a me issue or game issue or what, I know the filter used to work then at some point it stoped. I added code to work around it but I guess it is not working in classic for some reason.

commented

The API is all kinds of weird. The documentation I found does not match reality on Cataclysm Classic, and at least one method used in the filter's Matches does not exist at all.

While I've been looking through the code, I've also noticed another wart that should've popped up sooner: equipment sets are always character-specific, but Baggins profiles may not be and, at least for me, usually never are, so giving the equipment set filter that option is somewhat problematic. Would it be possible to make that part of the filter config character-specific, even if the profile itself isn't? Would that even make sense?

commented

FWIW, I think I can get the equipment set filter to work, but I'll hold off until the other current issues have been closed.

commented

I have reverted a change that triggered several issue I did not find in testing, please let me know if v5.4.6 works?

commented

v5.4.6 introduces a new LUA error when changing filter settings:

Baggins/Baggins-Filtering.lua:409: attempt to call method 'CategoriesChanged' (a nil value)
[string "@Baggins/Baggins-Filtering.lua"]:409: in function `OnRuleChanged'

In the meantime, I have rewritten the equipment set filter and got it to work with v5.4.5, except that the current bag layout isn't being updated on calling Baggins:OnRuleChanged(). Forcing that update by moving any item - can be one not involved in an equipment set, can be from any slot to any other inventory slot, can even be in the same bag - shows the items correctly filtered.

I'll open a PR in a minute.

commented
commented

I have fixed that error if you could test again?

commented

Yes, the LUA error is gone, but rule updates still don't trigger the refresh correctly, i.e. it still takes a BAG_UPDATE game event for Baggins to update.

commented

From what I noticed in testing function Baggins:Baggins_CategoriesChanged() calls function Baggins:Baggins_RefreshBags() and at least for the "temp disable cmopression" option bags did not change unless I commented out "sectionframe.dirty" part of function Baggins:Baggins_RefreshBags() . Not sure if this would help on category change or if its a "proper" fix.

commented

I'm going to actually play the game now for a bit, but after that I'll read through the refresh logic and see what I can find. After all, refreshes triggered from BAG_UPDATE events correctly update Baggins, so the codepaths for events and programmatic refreshes must diverge somewhere.

commented

Thanks for all your help, I have been trying to work on a few things and part of that is tracking down which functions get called where and I have noticed it is a bit of a mess and some times one function only exists to call another, making it harder to track things down, I have been trying to eliminate things like this.

commented

Tested the new filter code you PR'd on retail and cata, it only seems to suffer from the same not updating bag frames issue as others so closeing this and other issues that are similar to track in one place: #124