shagu / pfUI

A User Interface Replacement for World of Warcraft: Vanilla & TBC
https://shagu.org/pfUI
MIT License
333 stars 116 forks source link

hoverbind: add support for mouse button & mouse wheel bindings #1269

Closed Pizzahawaiii closed 3 months ago

Pizzahawaiii commented 3 months ago

Fixes 🐭 #1016 and 🐁 #1127

Summary

This extends hoverbind mode to allow binding mouse buttons and mouse wheel up/down to actionbar buttons. It does this by overlaying all actionbar buttons with separate invisible frames that receive all mouse and keyboard events and handle binding creation and removal.

Versions Tested

Tested and everything seems to work fine on the following clients:

Notes/Questions

  1. Against my own expectations, I actually managed to make this work somehow... 😄 Thanks again for the ingame support last night @shagu! ❤️
  2. Since I'm new to addon development, I'm not sure if this is the most elegant implementation. I'll let you be the judge and I'm looking forward to any kind of feedback.
  3. Do we even need the need_save variable & logic? The code would be nicer and simpler if we just always blindly SaveBindings(), even if there are no changes. Would that cause any (e.g. performance) issues? Update: I removed this so we always just save bindings immediately. (839a718)
shagu commented 3 months ago

This is great! Thank you very much for your contribution. :+1: I won't have much time today. But I will test and review your PR as soon as possible.

shagu commented 3 months ago

I have reviewed and tested it. It's looks very good, there is only one thing missing that I'd like to see:

I'd say there should be a safeguard to prevent users from binding the Left-Mousebutton by accident. The Left-Mousebutton is by default set to interact/action, which is a hidden binding in the 1.12.1 client:

  <!-- Hidden bindings -->
  <Binding name="TURNORACTION" runOnUp="true" hidden="true">
    if ( keystate == "down" ) then 
      TurnOrActionStart();                                                                                                                                                                 
    else 
      TurnOrActionStop();
    end  
  </Binding>

That means, people can't use the default keybind window to undo their change if they misclicked. The only option they'd have is to ask for support and then to macro their way out of the misery. (Or reset ALL keybindings and start all over again)

In my opinion, checking in hoverbind-code for a normal click (without modifiers) and ignoring the event would be the best solution here.

Pizzahawaiii commented 3 months ago

Thanks for the review!

You're right, we definitely don't want that. The thought had crossed my mind that binding left (or right) mouse button is kind of pointless. But I wasn't aware of the fact (and didn't consider the possibility) that binding left mouse button would render the player unable to use the UI, including the default menu, so I just left it there. Good catch!

Do you think we should also block the right mouse button in the same fashion, or merge as-is and leave that up to the player?

shagu commented 3 months ago

Do you think we should also block the right mouse button in the same fashion, or merge as-is and leave that up to the player?

Removing both left- and right button (without modifiers) sounds good :)

Pizzahawaiii commented 3 months ago

Alright, done! ✅

shagu commented 3 months ago

Awesome! Good job! Merged :)