moo-man / FVTT-DD-Import

Allows Importing Dungeondraft map files into FoundryVTT
MIT License
60 stars 28 forks source link

[FIX] Fixed old-school essentials support #117

Closed bakbakbakbakbak closed 2 years ago

bakbakbakbakbak commented 2 years ago

Renamed path variable to be compatible with old-school essentials system.

The "path" variable was superseeded from the OSE system, and resulted in "/systems/ose/dist[object Object]" as default upload path. With the changes it becomes the default again.

anthonyronda commented 2 years ago

as the system maintainer, I may rewrite our helper names to prevent conflicts with modules from now on. I hadn't realized this could be a source of conflict until today

I don't see a GH Issue so for your reference this is the issue that a user reported addressed in @bakbakbakbakbak's PR image

moo-man commented 2 years ago

This surprises me too, In what scope is it being overridden? I don't think the variable name needs to be changed throughout the entire module, I'm not inclined to mess up everyone's settings.

bakbakbakbakbak commented 2 years ago

The settings variable seems to be fine to keep, it is somewhere in the rest of the code it overrides, I'll keep checking how to minimize the changes.

bakbakbakbakbak commented 2 years ago

Found the culprit. It is the handlebar helper that is causing the issue. The OSE system has the following helper defined, this overrides the "{{path}}" in value of the html template when it renders the importer window.

Handlebars.registerHelper("path", function (relativePath) {
  return `${OSE.systemPath()}${relativePath}`;
});

With my latest commit, where I reverted the setting-name, it shouldn't break any current settings, and only work on the javascript variable names. I guess it may be minimized by just changing the scope where the variable is read for the template, but I know too little about that to do without trial and error :)

moo-man commented 2 years ago

I think the only change that needs to be made to fix this would be

<input class= "path-input" type='text' name="path" value="{{this.path}}"

this is also provided to handlebars probably for this specific reason, so {{this.path}} should work.

bakbakbakbakbak commented 2 years ago

Indeed it works @moo-man ! That is definitely minimum changes now, I tested it both with OSE and DnD5e to confirm it working!