Handling additional buttons in DrawingHUD

[hidden crest]

On a side note: I was able to replace all other occurrences of monkey patching in the module by mixins for Drawing, DrawingDocument, and DrawingsLayer. This seems to work fairly well (although it looses additional benefits that libWrapper provides, like ordering, and conflict detection).

I could imagine 2 solutions that avoid the need for monkey patching, while still not requiring people to write their own click handlers for buttons they put into the HUDs:

1. Make it possible to configure the used HUD classes via the CONFIG, something along the lines of

const CONFIG = window.CONFIG = {
  /* ... */
  HUD: {
    tokenHUDClass: TokenHUD,
    tileHUDClass: TileHUD,
    drawingHUDClass: DrawingHUD,
    chatBubblesClass: ChatBubbles
  }
};

or similar (alternatively, the HUDs could be defined as CONFIG.Token.hudClass etc. but it doesn't really fit for ChatBubbles...). This would allow people to use mixins to inject their functionality, instead of monkey patching.

2. Similarly to the getSceneControlButtons hook, provide get<HUD class>Buttons hooks (getDrawingHUDButtons etc.) that allow people to inject their own buttons with functionality into the HUDs.

[foggy pumice]

What duplicate functionality do you mean? Is there something more complex than just attaching an onclick event listener to the HTML?

[hidden crest]

No, not really.

Maybe I am making this more complicated than it actually is, but it just seems weird to to implement this as a separate thing, if there already is dedicated functionality. If it was handled completely separately, it would also be harder to understand if anything goes wrong, because you would now need to now that some of the buttons are being handled by _onClickControl, and some others are being handled by other on click handlers, added in the render hook.

I guess it just feels weird to use a completely different solution that what you would do if you extended the BasePlaceableHUD to create your own hud for whatever.

It would also mean I need a separate way to identify my own buttons, so there will be some small inconsistency between the buttons provided by core and my own buttons. And I hate inconsistencies 😅

Is adding custom on click handler during the render book really the recommended approach here? That always felt extremely tucked on and “workaroundish” to me in general, also for other applications I would@much prefer to inject some functionality into activateListeners, where activating click handlers actually belongs…

[hidden crest]

OK, I tried it out, and it isn't actually that bad. So maybe that's really to way to go here.

[compact tinsel]

In general, using the render hooks and attaching listeners there is pretty acceptable behaviour for modules. Something like getDrawingHUDButtons doesn't sound like a bad idea either though.

[foggy pumice]

Here's an alternative frame of reference that might help:

Say you implement your click handler incorrectly, and it throws an error. Which is more clear and helpful to the user - seeing that error came from GhostsCoolModule#_onClickControl, or from the core one instead?

You can still use the same classes, adding additional handlers and having the core one run as well on click is super cheap - it'll execute that switch statement and cleanly return in mere milliseconds

[hidden crest]

well, with libWrapper, there will be logs that clearly show that the error came from my wrapper, explicitly mentioning the module. Similarly, if extended the class, overriding the method, the stack trace would show that it threw the error in my class

[foggy pumice]

ah right, I forgot Libwrapper could do that

[hidden crest]

just blank monkey patching is pretty bad, of course 🙂

[foggy pumice]

still, having your own handler is a clean separation of concerns IMO, but I understand the appeal of trying to share a single handler - in this case, I don't think that second way is the way to go though

[hidden crest]

but I think it's fine, having fiddled around with it more, I kinda like the approach of adding my own handler because it keeps the code of injecting the html and adding the handler in a single place.

and I manually had to call activateListeners again with the old solution, because injecting stuff in the render hooks happens after activateListeners is called by _render. So I guess this solution really is more clean

thanks for the feedback everybody!