Factor out onButtonEvent method from controlers

[dmarcos]

It factors out the onButtonEvent method common to all controls. Other methods can be also factored out like onButtonChanged or onAxisMoved. I will do follow up PRs with those. I started with onButtonEvent for easier review

[ngokevin]

Is there a plan to eventually make this a registerController API? Otherwise it might eventually become hard to follow code if they all implement a tangle of utils.

[ngokevin]

There's a tracked-controls utils where all the controllers share code already

[dmarcos]

It makes sense for me to separate tracked-controls from the higher level controls utils

[ngokevin]

Don't need the array handler?

[dmarcos]

I don’t think we need it. I don’t see it used anywhere. I believe we went ahead of ourselves and thought we would neef multiple events when a button is pressed. I can reintroduce but I couldn’t find where this code path is used.

[machenmusik]

I believe that was originally there for buttons that were aliased to trigger more than one named event (e.g. 'menu' and 'app') but then we resolved the naming such that there were no duplicates. So it should not currently be used anywhere

[dmarcos]

I think is a bit premature to do a registerController API. There’s plenty of ad-hoc logic to deal with the specifics of each controller and I don’t feel ready for abstraction. Factoring out a few functions will clean up and make code testable. We can reevaluate anregisterController API a bit later.

[ngokevin]

We have controlsUtils and controllerUtils, could be confusing

[ngokevin]

Concerned about passing in component and having a utility call a bunch of the component's methods.

For example, we did this with checkControllerPresentAndSetup, I found it hard to follow. The utils depends on component.el, component.controllerPresent, component.addEventListeners, component.controllerEventsActive, component.injectTrackedControls, component.removeEventListeners. There's a huge amount of interface expected to be available.

Since we want to make available these shared methods and have the controllers implement this common interface, we might want to think early about a higher level API/prototype mixin early. Splitting out to a util makes it seem refactored, but the util and component are heavily coupled and not any more testable. It might be the wrong road to go down.

[machenmusik]

Agree that although the attempt to refactor into utils allowed code reuse, it made it nearly impossible to understand what things were doing without a pretty thorough look

[dmarcos]

You're correct. It's essentially a mixin but passing the this as an argument rather than this.foo(). I want to stay focused on the task of improving controls. My goal is to easily be able to add thumbpadup, thumbstickup methods without having to copy code across controllers. There will be just three methods factored out from controllers. In a separate PR we can think of a mixin mechanism that consolidates checkControllerPresentAndSetup and these factored out methods in a more sustainable pattern.

[ngokevin]

I'm not sure the utils improves the code quality of the controls since it makes things hard to understand, I'd favor copy and pasting code. Ideally, if we're refactoring, we might as well refactor in the right direction. We can merge something temporary if we agree on the mechanism and someone promises to do it soon.

[ngokevin]

Perhaps not agree on the specific code, but that we can have a mixin-style refactor instead soon after you are able to finish the thumbpad/stick work. Then mostly OK with something temporary, these methods are at least less coupled than the tracked-controls utils.

[ngokevin]

OK, we can go ahead. It's a sideways move, we can do a bigger refactor later.

[dmarcos]

I consolidated the new methods in tracked-controls utils