Merchant Plus - Improved Vendor UI

Merchant Plus - Improved Vendor UI

3.3k Downloads

[question] Addon conflict causes wrong items to be displayed at merchant

tflo opened this issue · 12 comments

commented

I noticed that sometimes Merchant Plus doesn't show the actual inventory of the current merchant, but the inventory of the previous merchant.

For example, yesterday I went to the DMF to test the Flimsy Yellow Balloon for the other issue: When I clicked on the balloon vendor, Merchant Plus showed me the inventory of the vendor at the DMF entrance in Goldshire, where I had been before to buy the Simple Flour.

I had to reload to get the actual merchant shown in Merchant Plus. Then I tried without any other addons and could not replicate the behavior.

I usually have many addons loaded, and it probably won't be trivial to isolate the culprit addon combination. So, just a quick question:

Do you have any idea what an addon must be doing to cause this behavior? This could probably help me find the culprit.

It's like MP would use some cache instead of actually querying the current merchant inventory.

Thanks in advance.

commented

PS: The "normal" (Blizz) merchant display shows the correct items.

commented

I have not forgotten this, but have not been able to replicate it again recently. Probably because since 10.2 I haven't seen lags (mailbox etc.) as bad as before.

If you don't like open issues, feel free to close this; I'll post as soon as I see the problem again.

commented

I've never seen this before. I'm not formally caching anything, but the data structure of the item list is a persistent object fed into Blizzard's TableBuilder. That data should get refreshed any time the merchant frame is opened and upon other key events. If an addon is changing the default behavior when opening the merchant frame, I might not detect it and fail to call the refresh function, which would cause it to show the last state of the window.

It might be helpful to enable to trace messages in the chat window and see what's different between when it works and when it doesn't.

You can turn that on by running:

/run MerchantPlus.Trace = true

You can turn it off by setting the same value to false.

If you find a specific add-on responsible, let me know and I'll take a look.

commented

Just for some extra clarity, the main places where refresh occurs: function MerchantPlusItemListMixin:OnShow() and events MERCHANT_UPDATE and UNIT_INVENTORY_CHANGED.

When the merchant is closed it should fire the event MERCHANT_CLOSED which triggers hiding the frame with MerchantPlusFrame:Hide(), and which should then allow OnShow() to get called next time the tab is shown. If something is getting in the way of that, it could be the culprit.

commented

Thanks for the info and the trace toggle. That will certainly help.

However, currently I'm unable to reproduce it: exactly same addon set as yesterday, and with the same vendor (the DMF balloon gnome).

If it is there (as it was yesterday), then it is endlessly repeatable: I could reload, summon and open my Expedition Yak merchant, click the other merchant --> MP showed the Expedition Yak merchant items on the other merchant; reload again… rinse and repeat.

(A preceding reload is not a condition for the phenomenon, I just reloaded to reset the situation so that I could try again. It's not the heavy load after login/reload that is causing it.)

The previous time I saw this, it was a different toon and different merchant.

Maybe it has to do with server conditions? Yesterday I had horrible mailbox lag1, today it's less pronounced. Just thinking…

Footnotes

  1. Mailbox lag: clicking Send button sends after a delay of 5s or so, or not at all, or only after I move the to-send item or some item in my bag. Lag comes and goes; mostly better or non-existent after weekly server restart.

commented

Maybe it has to do with server conditions? Yesterday I had horrible mailbox lag1, today it's less pronounced. Just thinking…

So I think there's a possible race condition involved here triggered by lag.

In order to make sure that I don't waste cycles, if the item list isn't visible I don't do anything when a refresh is called for. One main reason for this check is that the event UNIT_INVENTORY_CHANGED gets fired all the time and can represent events that require a merchant refresh. If no merchant is active, it's wise not to call any merchant APIs. This check also suppresses updating data is if the Merchant Plus tab is not selected. I only fill in the data when the tab becomes active.

The logic actually depends on the following things to happen, roughly in order:

  1. MERCHANT_SHOW event is fired. If the pref says to open Merchant Plus by default, I set a bool SwitchOnOpen to true

  2. When MerchantFrame appears, it calls its own MerchantFrame_OnOpen which sets the tab to the normal Merchant tab

  3. If SwitchOnOpen is true, when the tab is switched, I then switch the tab to Merchant Plus instead, clear the bool, and call Addon:UpdateFrame()

  4. Addon:UpdateFrame() does a bunch of frame setup and then calls MerchantPlusFrame:SetShown(true)

  5. If the item list is actually visible, not just marked shown, it will refresh the data for the tab from the API and generate the table

So, if my addon believes its tab is being set to active before MerchantFrame actually itself becomes visible, my Addon:UpdateFrame() function might call MerchantPlusFrame:SetShown(true), but skip the refresh. When the MerchantFrame finally shows up, it will not fire my OnShow() function again because it was already fired, leading to no refresh ever being called.

In theory, moving something around in your inventory with the merchant open should cause the refresh, in that case.

As to why Blizzard's own tab has the right data, MerchantFrame_Update() does a bunch of setup to the frame regardless of whether it's visible because they just assume no one is replacing the merchant UI, which is why you end up seeing the regular tab with the correct contents.

As to how an addon could cause this behavior... The MerchantFrame only sets the tab in MerchantFrame_OnShow() which shouldn't be called before the whole frame is actively being shown. Since MerchantFrame is the parent to everything else, it shouldn't normally be possible for it to be both invisible and shown, like child frames can be. That said, the IsVisible() check tests for the real visibility of a frame. If the base game UI is lagged, either through addon misbehavior or server issues and the frame fires OnShow() but doesn't become visible within a cycle or two, there might be a moment when I set my tab active and due to its invisibility, skip the refresh.

I'm not sure what the right solution would be to fix this, and without a way to reproduce it reliably it's hard to try out some ideas. I don't want to switch from using IsVisible() to IsShown() because technically the children of MerchantPlusFrame are always shown and I'm just hiding and showing their parent frame. I also don't want to refer to MerchantPlusFrame or MerchantFrame directly from item list code because I'm trying to keep that code agnostic of what frame it's used it in case I want to reuse it in another interface.

commented

Thanks for the details.

If the base game UI is lagged, either through addon misbehavior or server issues […]

I guess these two things are adding up:

  • On the laggy day, I had the issue, but could mitigate it by unloading addons
  • On the less laggy day (yesterday), I was unable to reproduce it, even with the full addon set

If this is true, then there is no "culprit" addon combination.

In theory, moving something around in your inventory with the merchant open should cause the refresh, in that case.

Yes, after I wrote that moving an item can nudge my lagging mailbox to actually send, I decided to test this at the next opportunity.


On a sidenote:

With the MerchantPlus.Trace enabled, I noticed that MP is constantly responding to UNIT_INVENTORY_CHANGED, even when no merchant is in sight. For example, while fishing:

You receive loot: [Darkmoon Daggermaw]
You receive loot: [Great Sea Herring]
called: UNIT_INVENTORY_CHANGED
called: RefreshScrollFrame
You receive loot: [Darkmoon Daggermaw]
You receive loot: [Great Sea Herring]
called: UNIT_INVENTORY_CHANGED
called: RefreshScrollFrame
You receive loot: [Darkmoon Daggermaw]
You receive loot: [Great Sea Herring]
called: UNIT_INVENTORY_CHANGED
called: RefreshScrollFrame
You receive loot: [Darkmoon Daggermaw]
You receive loot: [Great Sea Herring]
called: UNIT_INVENTORY_CHANGED
called: RefreshScrollFrame

Wouldn't it be better to unregister this event (and possibly other events) when the merchant is closed, and re-register it/them on MERCHANT_SHOW ? (Not trying to tell you how to write your addon, just something that caught my attention).

commented

Unregistering the event could be done, though as noted my current code ignores the refresh if Merchant Plus list isn't visible, such as when the merchant is closed, so it shouldn't be doing any actual work. I should probably log a message to this effect to help with debugging.

Overall I need to refactor some of this code to fix compatibility with Scrap, so I'll try to consider some more robust handling of the refresh when I do that.

commented

No luck replicating the issue yesterday, and today the server seems to be completely lag-free :(

commented

Just a lil bump, still working. I'm observing it, but …

commented

I still haven't encountered this myself and haven't had a chance to work on the code refactor I was planning, so I don't have anything new to share, at least from my side.

commented

Closing this for now, as it really seems to be a side effect of a stupidly high server lag, which – hopefully – is a thing of the past. Probably there are precautions you could take to smooth out the behavior under such circumstances, but I don't think it deserves being an open issue.
For the record: It was only reproducible for one or two server weeks and did not happen again for almost 3 months.