manuelVo / foundryvtt-drag-ruler

A Foundry VTT module that shows a ruler when dragging tokens so you can see how far you've dragged them
MIT License
39 stars 50 forks source link

Does not wrap methods properly for use with libWrapper #310

Open caewok opened 7 months ago

caewok commented 7 months ago

I think this forwardIfUnhandled function in main.js is breaking libWrapper compatibility:

function forwardIfUnahndled(newFn) {
    return function (oldFn, ...args) {
        const eventHandled = newFn(...args);
        if (!eventHandled) oldFn(...args);
    };
}

Basically, I am unable to wrap Token.prototype._onDragLeftDrop or (with dnd5e) Token5e.prototype._onDragLeftDrop. Wrapping the latter never gets called at all, and wrapping the former completely fails to call the libwrapper wrap chain properly.

This function should be unnecessary when using libWrapper and should probably be removed completely, or at least used only when libWrapper is not present.

This primarily affects "Elevation Ruler" but also may affect compatibility with my other modules. I know that I get a lot of issues related to Drag Ruler compatibility, and I don't think I can fix them as it stands now.

thatlonelybugbear commented 7 months ago

Glad to see this issue cause I was banging my head, trying to wrap Token.prototype._onDragLeftDrop for another module and failing spectacularly. Now the behaviour I encountered makes more sense.

manuelVo commented 5 months ago

This function is necessary, because when Drag Ruler is running it needs to block foundry from executing it's native Drop handler (if it was executed it would cause bugs). Essentially, what this does is: It checks, if the currently dragged token has a Drag Ruler attached. If so, Drag Ruler performs whatever is necessary to provide Drag Ruler functionality. If not, the call is forwarded to foundry for default handling. To avoid duplicate code, the wrapping functions simply return a bool to indicate if the event has been handled (which means further hooks will be blocked) or if the event hasn't been handled, and the outer function then takes part of forwarding the call accordingly. To avoid having to write the outer function again and again, this construct exists: A function, that generates the "outer function" which, when called, will first call the wrapping function and if that returns false (meaning the event hasn't been handled because it's an event that Drag Ruler doesn't have anything to do with) then the old function will be called.

What's true is that this might block other modules from executing their function handlers in some cases. There are two main ways to remedy this:

  1. If you don't need to block the native foundry event (which it sounds like from what you're writing) you should register your hook in libwrapper as WRAPPER. That way, your code will always be executed before Drag Ruler's hook is executed (because Drag Ruler registers as MIXED which has lower priority). As a Result, Drag Ruler will not be able to block your code.
  2. If 1 isn't possible, and you need to register as MIXED as well, the priority (which hook is executed first) can be adjusted in the libwrapper settings. By increasing your module's priority, you ensure that you're always called before Drag Ruler is.

Unfortunately, I don't think that I can get rid of the quoted function, but if you're having any concrete examples of breakage, please point me towards them. I'm glad to see if I can help come up with a fix.