Fix key bindings

[vaindil]

Just a note: This is my first time ever using Lua or doing anything with a WoW addon.

Key bindings set with default="key" in Bindings.xml aren't persisted after a UI reload, so I wrote this code to work around that. It saves them as actual bindings as long as:

• the user hasn't previously set their own bindings for the addon, and
• the default bindings aren't already used anywhere else.

I don't know if you'd like to display a message if either of those is false (so bindings aren't set), but I didn't add them because presumably the bindings have never worked if that's the case. I know the comments are a little gratuitous, let me know if you want those changed (or you can change them).

I also removed runOnUp="true" from Bindings.xml, because that attribute causes the action to run both when you press the binding and when you release, so the handlers were unnecessarily running twice (source).

[NielsHeltner]

Thank you for the pull request! Have you tested if it actually works ingame?

I've attempted something very similar in the past, but ran into the same issue your approach seems to have: calling the SetBinding function (and SaveBinding) manually ingame works as expected. However, calling SetBinding from an addon looks like it succeeds, as it returns true, and subsequent calls to the GetBindingKey function returns the correct keybind, however the keybind doesn't actually work when you press it, and the keybinds UI doesn't get updated.

[vaindil]

Yes, I tested each part of this pretty extensively. I found that having the default attribute in Bindings.xml prevented it from saving at all (even though it said it succeeded), which is why I removed it, but I'm actively using my fork at the moment and it has worked perfectly.

I wrote the part that does the bindings first without the saved variable part, and that's easy to test since you can just unbind the keys through the UI and /reload to trigger it. I then tested the saved variable logic by setting the property to nil inside the slash command handler to clear it, which also lets you do a /reload afterward to test.

[vaindil]

I've uploaded a testing video. It uses the code in my branch named fix-bindings-debug. The only changes between that debug version and the PR version are that calling the slash command sets WowheadQuickLinkCfg.defaultBindingsSet = nil for easy testing, and a bunch of debug messages are added to the default chat frame (diff is here). I have no other addons enabled in the video (which is why my UI looks weird, heh).

I just realized I forgot to test in Classic, maybe that's what you were talking about. Setting bindings doesn't seem to work in Classic, I'm looking into it.

[vaindil]

Okay, I pushed two commits to both branches. The first fixes my PR in Classic (Classic uses AttemptToSaveBindings instead of SaveBindings). The second fixes an apparent bug with the addon in Classic unrelated to my PR; trying to use the key binding on anything outside of the AH would throw a Lua error.

Sorry for three comments in a row!

[NielsHeltner]

It might be another addon blocking the SetBinding call for me.

However, here is a test video on a trial character with no other addons enabled, using your debug branch. That character doesn't have character specific keybindings enabled. If I enable the addon and log in (with WowheadQuickLinkCfg.defaultBindingsSet being nil), it would seem like the it changes to character specific keybindings, and resets all the keybindings on it (and if I relog or reload from here, keybindings are restored).

[vaindil]

That's very strange behavior, but I was able to reproduce it with a fresh WoW account and character. After much troubleshooting, it looks like ADDON_LOADED is just too early to deal with bindings, and switching to PLAYER_LOGIN fixes the problem.

[NielsHeltner]

Terribly sorry about the long response time, but it looks good now!

Thank you for the contributions, I'm very happy to see this bug fixed.

[vaindil]

Perfect, thank you!