AngryBeaver / beavers-crafting

Crafting module for Foundry
MIT License
8 stars 3 forks source link

Tidy 5e Sheets rewrite compatibility #98

Closed kgar closed 10 months ago

kgar commented 10 months ago

I am working on the rewrite of Tidy 5e Sheets (new version here, and I am hoping to establish compatibility with this module. The new sheets are running on svelte, so the render lifecycle is different from a standard Foundry document sheet, and the means of registering tabs is also different because I offer features like tab selection to the users.

Would it be possible to work together on ways to integrate this module with the new Tidy? I have APIs for registering tabs, and I can create more APIs/hooks to fill in anything that is missing.

AngryBeaver commented 10 months ago

hm this would need some refactoring i guess and possibly break existing apis. While ofc i think i would manage it to make an if condition and seperate things . I would prefer not to add system-dependend code into beavers-crafting. -> means i would need this to end up somehow in bsa-dnd5e which is the adaption layer for dnd5e and implements the actorSheetAddTab method for this system. But that probably won't be enough. As beaverscrafting is making use of the app._tabs( which it should not and is dirty i know) and that is where refactoring starts. In the end it has to work on all other systems aswell as for your sheet and dnd5e without your sheet. I am looking into this a bit, but as it seems complex it is currently of low prio.

AngryBeaver commented 10 months ago

I haven't created own sheets yet but wouldn't it be possible to still use the default tab system then it keeps more compatibility to other modules beside the fact that my implementation is not great.

You want your tabs to come from settings wouldn't it be possible to add those tabs in your static default method ? https://github.com/kgar/foundry-vtt-tidy-5e-sheets/blob/main/src/sheets/Tidy5eCharacterSheet.ts#L73. Then they might get picked up by foundries: _createTabHandlers() method but i am unsure of how the timings are working.

on the fly changes in your settings however then would need to destroy all sheet-apps and recreate them or force a relaod.

AngryBeaver commented 10 months ago

yeah no thats not how that works :-)

kgar commented 10 months ago

I know this is not 100% the answer, but as a proof of concept, I was able to get something to show up on the Actor sheet by registering an HTML tab via my API.

World script:

import { Settings } from "/modules/beavers-crafting/src/Settings.js";
import { ActorSheetTab } from "/modules/beavers-crafting/src/apps/ActorSheetTab.js";
import { ActorSheetCraftedInventory } from "/modules/beavers-crafting/src/apps/ActorSheetCraftedInventory.js";

Hooks.once("tidy5e-sheet.ready", (api) => {
  api.registerCharacterTab(
    new api.models.HtmlTab({
      html: '<div class="sheet-body"></div>',
      title: "beaversCrafting.actorSheet.tab",
      tabId: "beavers-crafting-tidy-actor-tab",
      tabContentsClasses: ["beavers-crafting"],
      enabled: (data) => !Settings.isDisabledActor(data.actor),
      onRender: ({ app, element, data, isFullRender }) => {
        const html = $(element);
        if (!isFullRender) {
          if (!Settings.isDisabledActor(app.actor)) {
            new ActorSheetTab(app, html, data);
          }
          new ActorSheetCraftedInventory(app, html, data);
        }
      },
    })
  );
});

image

image

One thing to note about my API: because it is being used within a onetime hook subscription, if my module is not installed, none of the logic fires. However, this is the kind of script that only works if Beaver's Crafting is Active, because of how I wrote it, but I imagine there are ways to mitigate that. Even so, there are probably some users on my side who would add a world script just to use our two modules together.

There are some obvious problems with this particular proof of concept. If I put a .sheet-body div in multiple tab content areas as a shim/target, other modules will potentially be rendering into each other's tab areas. There's also a harmless console error when it tries to call activateTab():

image

kgar commented 10 months ago

I tinkered some more and worked out

// World Scripter integration; they execute the pack of macros after Foundry "ready" has fired
const api =
  game.modules.get('tidy5e-sheet')?.api ??
  game.modules.get('tidy5e-sheet-kgar').api;

if (api) {
  applyCompatibility(api);
} else {
  Hooks.once('tidy5e-sheet.ready', async (api) => {
    applyCompatibility(api);
  });
}

async function applyCompatibility(api) {
  if (!game.modules.get('beavers-crafting').active) {
    return;
  }

  const { Settings } = await import(
    '/modules/beavers-crafting/src/Settings.js'
  );
  const { ActorSheetTab } = await import(
    '/modules/beavers-crafting/src/apps/ActorSheetTab.js'
  );
  const { ActorSheetCraftedInventory } = await import(
    '/modules/beavers-crafting/src/apps/ActorSheetCraftedInventory.js'
  );

  function createCraftingTab(api) {
    return new api.models.HtmlTab({
      html: '<div class="sheet-body scroll-container"></div>',
      title: 'beaversCrafting.actorSheet.tab',
      tabId: 'beavers-crafting-tab',
      tabContentsClasses: ['beavers-crafting'],
      enabled: (data) => !Settings.isDisabledActor(data.actor),
      onRender: ({ app, data, tabContentsElement, isFullRender }) => {
        // prevent tab activation console errors on my sheets
        for (let tab of app._tabs ?? []) {
          tab.activate = () => {};
        }
        const html = $(tabContentsElement);
        if (!isFullRender) {
          if (!Settings.isDisabledActor(app.actor)) {
            new ActorSheetTab(app, html, data);
          }
          new ActorSheetCraftedInventory(app, html, data);
        }
      },
    });
  }

  api.registerCharacterTab(createCraftingTab(api));
  api.registerNpcTab(createCraftingTab(api));
  api.registerVehicleTab(createCraftingTab(api));
}
kgar commented 10 months ago

I haven't created own sheets yet but wouldn't it be possible to still use the default tab system then it keeps more compatibility to other modules beside the fact that my implementation is not great.

You want your tabs to come from settings wouldn't it be possible to add those tabs in your static default method ? https://github.com/kgar/foundry-vtt-tidy-5e-sheets/blob/main/src/sheets/Tidy5eCharacterSheet.ts#L73. Then they might get picked up by foundries: _createTabHandlers() method but i am unsure of how the timings are working.

on the fly changes in your settings however then would need to destroy all sheet-apps and recreate them or force a relaod.

I unfortunately cannot do seamless compatibility with tabs or with other sheet augmentations. The render lifecycle of my sheets is long-lived with fine-grained reactivity. It offers great performance, nice animations, and some major benefits to maintaining the codebase and making new features. However, it means any stateful content that is injected in will not re-render until there's a forced, full re-render on the sheet. For these reasons, it's generally better not to receive accidental injections of content from other modules. As a result, the incompatibility is by default. That is on me, because of my design decisions. As a result, I have been working to build APIs and creating my own extension points in my sheets.

I understand how and why this module is system agnostic after reading through some of the related repo readmes, and I wonder what might be a good solution. For as long as I need to, I can give my user base a world script, but there are some issues with that approach, of course.

With Foundry 12 coming and potentially FormApplication V2 on the way, we may need a lot more API-based integration, since V2 is supposedly going to be less opinionated on the HTML structure. I wonder how long before some sheets start using V2 and what that implies. I'll stop rambling now 😅

AngryBeaver commented 10 months ago

Yeah need to test v12 anytime soon and if they change the FomrApplication i hope they come up with more official hooks i can make use of instead of what ever that is i am doing right now.

I will pollute my beavers-crafting with your hook solution for now to honor your effort you had with this and make it simpler for users.

A problem though is that my module is basically unfunctional when the tab is not shown and your module can hide the tab but what is worse hides the tab initially so users of your module need to know they can enable or disable tabs and need to know that they have to enable a tab in order to make my module work on your sheet or wont see anything and migth not know they need to do something.

I probably will write something that on the hook.once will attache my tab to your settings so ... any time you reload my tab is on your sheet :-) and to disable that they would need to disable beavers-crafting hiding the tab would effectivly doing the same.

Or maybe you have a better idea about that problem.

kgar commented 10 months ago

I'll puzzle over it for a while. I could have a special tooltip that appears when the registered tab includes an enabled callback which states that the tab has conditions for appearing on the sheet. That would probably be good enough.