AraBroker / Ara_Broker_Reputations

Continuation of Ara_Broker_Reputations WoW Addon
1 stars 0 forks source link

[bug] Collapsed/expanded states are not robustly saved #6

Closed tflo closed 1 year ago

tflo commented 1 year ago

I noticed that often my collapsed/expanded states of the expansion groups are not saved or get lost. But sometimes they are.

So, tried to find out what is going on and tested it on 2 chars, and it is weird:

Char A:

  1. Changed the expanded/collapsed states to my liking.
  2. Reloaded --> States were saved correctly.

Char B:

  1. Changed the expanded/collapsed states to my liking.
  2. Took the portal from Bastion to Oribos --> States were lost (reset to how they were at login)
  3. Corrected the states again.
  4. Jumped from Oribos into the Maw --> States were lost again
  5. Corrected the states again.
  6. Talked to Venari and turned in the weekly, i.e. rep has changed --> States were lost again.
  7. For testing, just talked to Venari again and accepted some random quest (no rep changes) --> States were not lost.
  8. Back to Bastion via Hearthstone. --> States lost again.

So, it seems the states get lost with PLAYER_ENTERING_WORLD, but what with the rep change at Venari? Rep changes should not reset the collapsed/expanded states either.

– Tom

tflo commented 1 year ago

PS:

Btw, a function to mass expand/collapse all groups would be nice. For example shift-clicking any Collapse button.

tflo commented 1 year ago

PS2:

On a third char, even more weird:

  1. Logged in and all groups were expanded.
  2. Collapsed all groups except Shadowlands.
  3. Reloaded. --> Correctly saved.
  4. Took portal to Oribos. --> Now all groups from top till down to Pandaria were expanded again. But not the ones further down!
mightykong commented 1 year ago

Are you using Altoholic by chance? If you are, that is the mod that is not respecting collapsed state. The problem is in their DatatStore_Reputations.lua module where they are ignoring collapsed state and setting everything to uncollapsed.

tflo commented 1 year ago

Yes, I'm currently using Altoholic.

Didn't know that Ara is using their DatatStore_Reputations module. Then I guess you will suggest to wait for a fix on their side, right?

This is unfortunate however, because Altoholic is pretty buggy in general, so I wouldn't have big hopes that they fix that anytime soon.

If Ara really depends on Altoholics DataStore stuff, then I would suggest to get rid of that dependency as soon as possible, because, honestly, with non-saving collapsed states the addon is not really usable.

Ara is such a good rep broker mod, and it would be a great pity to see it destroyed by bugs in dependencies.

mightykong commented 1 year ago

I'm not, and there is no dependency. The problem is that the IsCollapsed state is saved as part of the Blizzard Reputation API state. It's gathered from GetFactionInfo, which is a Blizzard function. So, I set it to true if you collapse it, so that is shows collapsed not only in Ara, but also in the Blizzard reputation panel.

So, then Altoholic comes in and when it scans reps, it just sets IsCollapsed for reputations to False. This is not an Ara problem, it's an Altoholic one. And you can test this by collapsing the reps in the default Blizzard rep panel and then check it under the same circumstances.

tflo commented 1 year ago

Yes, I know that the collapsed states in Blizz's module aren't saved either, but, are you saying that the non-saving states in Blizz's rep module are also caused by the interference of DataStore?

I'm using Altoholic/DataStore only since DF prepatch, but I'm pretty sure that Blizz's module never saved the states correctly for me (similar to their tradeskill frame).

It's gathered from GetFactionInfo, which is a Blizzard function. So, I set it to true if you collapse it, so that is shows collapsed not only in Ara, but also in the Blizzard reputation panel.

This sounds like a good thing. But instead of reading the states back from GetFactionInfo, couldn't you simply read them back from Ara's database? This way you wouldn't be affected by any corruption that happens (via DataStore or whatever else) to the collapsed states from GetFactionInfo.

mightykong commented 1 year ago

I'm respecting and treating user choice based on the way user sets it - irrespective of whether or not they change it in Ara or on the Blizzard reputations panel and also reading and writing to the API that Blizzard has for this.

Altoholic is the one that is breaking it by ignoring user choice and just flatly setting isCollpased to false. That mod is ignoring the proper API. Could I code around it and ignore the API and create an isCollapsed that is unique to Ara? Perhaps. But not sure I should be ignoring the built in API for this.

I'm not trying to sound combative, because you're not the first one to post this as an Ara bug. But it truly isn't. The Ara code is honoring the API that was created for this, and personally IMHO, it should.

If I do that, there will be users that are upset because it's not honoring the API state. So, it's a double edge sword. I'm choosing the lesser of two evils here and sticking to the API state. It's up to Altoholic to validate why they are disregarding it and fix it. I don't want to get into the business of coding around problems in other mods because they're not using the proper APIs.

And -- to answer your question, yes I'm saying that Altoholic is breaking isCollapsed in the Blizzard UI, because it's blanket setting everything to isCollapsed = False

tflo commented 1 year ago

Just did a couple of tests. In my setup there is another factor in place: The rep module of Broker_Everything.

While DataStore seems to reset the GetFactionInfo collapsing in some weird patterns (and not always), Broker_Everything does completely uncollapse all after every reload or login.

This explains that in my perception "Blizz's module never saved the states correctly", because I was using Broker_Everything's rep module for years. Damn, I blamed Blizz all wrong (this time). Shame on me ;)

Considering that Blizz's stuff works correctly in this case, my "instead of reading the states back from GetFactionInfo…" proposal is pretty moot.

Of course, by doing so, you would be safe from corruption by DataStore/Broker_Everything (and maybe others), but you are right, you would be disrespecting a correctly working API then.

So, if I want a correctly working Ara (and Blizz module), I'll indeed have to ditch Altoholic. The good thing is that I considered it either way, since I used Altoholic as a replacement for Armory, which went out of service with the DF prepatch, but is now fully functional again. (In general, it's much better than Altoholic, but Altoholic had some minor things that made me keep it alongside Armory, till now.)

mightykong commented 1 year ago

Well, it's entirely up to you. I personally don't use collapsing that much. If it's important to you, you'll have to weigh its importance vs mods that don't honor it. But I've been down this road before troubleshooting Altoholic, and I'm not inclined to make fixes for them. ;)

tflo commented 1 year ago

and I'm not inclined to make fixes for them.

Sure, wouldn't do that either. (Just uninstalled it, btw ;)

I'm sorry to have bothered you with a completely misaddressed bug, and thanks for pointing me in the right direction!