DFreds / dfreds-convenient-effects

A FoundryVTT module that adds easy to use toggleable active effects for any system.
MIT License
45 stars 39 forks source link

close 263: Add a button to item cards to quickly apply effects #266

Closed kaelad02 closed 1 year ago

kaelad02 commented 1 year ago

Using a renderChatMessage hook, add a button to item cards if the name of the item has a matching convenient effect. Since this would interfere with Midi QOL's integration, there is a setting to control this which is off by default. I thought about making it a role-based config setting like APP_CONTROLS_PERMISSION is but it didn't seem necessary. In my world script, it was a client setting but it makes more sense here as a world setting. I even thought about checking both this setting and APP_CONTROLS_PERMISSION before adding the button but it seemed overkill.

DO NOT merge it yet, however. There is a timing issue I'm facing and could use some input on. I'll add a comment w/ that info.

kaelad02 commented 1 year ago

I originally wrote this as a world script on v9 and it looks like there were some changes since then (not surprisingly). After turning the setting on, I can use an item and it'll show the button correctly. If I refresh the browser I get error messages during the renderChatMessage hook because the EffectDefinitions instance has not been initialized yet.

TypeError: Error thrown in hooked function '' for hook 'renderChatMessage'. this._all is not iterable
[Detected 1 package: dfreds-convenient-effects]
    at get all [as all] (effect-definitions.js:44:39)
    at EffectInterface.findEffectByName (effect-interface.js:54:32)
    at Object.fn (main.js:298:37)
    at #call (foundry.js:730:20)
    at Hooks.call (foundry.js:712:38)

I added some temporary log messages to the hooks to get a better idea of the timing. It looks like this is the order of the hooks:

The game.dfreds.effectInterface and game.dfreds.effects variables have been instantiated in the socketlib.ready hook but they are not initialized until the dfreds-convenient-effects.initialize hook. My new renderChatMessage, which runs in the middle of those two, needs both to be initialized. Why are they initialized so late and is it possible to move that up?

While it's possible to change my code from using a renderChatMessage hook to a dnd5e.preDisplayCard hook, that would permanently add the button to the chat message data. It'd be there even if someone turns off the feature or disables the module entirely. I like it being temporary with the renderChatMessage implementation and would rather keep it that way.

DFreds commented 1 year ago

Hm, so it seems like an easy fix would be to change my init from ready to setup. That seems to make more logical sense anyway, and as far as I can tell makes no difference in the functionality. That would cause the new order to be

At least I think. Feel free to make that change in your PR and see if it works. In main.js, line 40. You could also try after init? Again, as long as everything works I'm not too hung up on where that stuff happens.

DFreds commented 1 year ago

Otherwise I approve the code changes. Good stuff. Let me know if the other issue is fixed

kaelad02 commented 1 year ago

Calling the hook in setup did the trick.