Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Tidy 5e Rewrite Compatibility #148

Closed
kgar opened this issue Jan 2, 2024 · 11 comments
Closed

feature: Tidy 5e Rewrite Compatibility #148

kgar opened this issue Jan 2, 2024 · 11 comments

Comments

@kgar
Copy link
Contributor

kgar commented Jan 2, 2024

I am the author of the Tidy 5e Sheets rewrite. The rewritten sheets run on svelte and have some significant differences from the default sheets and even original Tidy. With that in mind, I am requesting to work with you on a way to restore compatibility with Tokenizer.

Details

At present, the incompatibility is with my actor sheet portraits. They are using mousedown instead of click, and they do not have the [data-edit="img"] attribute in place. These are implementation details of my rewrite, but as an exercise, I lined things up as expected to see how far that would get me. I found that, because it's wired with svelte and not jQuery, $(element).off("click") doesn't affect my event bindings. As a result, shift+clicking causes 2 file pickers to appear: my original one and the one from the else case when the module decides whether to launch tokenizer.

One idea I have is to use a pre hook:
Like dnd5e's preUseItem hook, I could provide a hook that essentially provides the ability to hook into and prevent the default behavior when someone interacts with a sheet portrait, one hook for actors and one for items. Something like tidy5e-sheet.preOpenActorPortraitFilePicker with params (actor, event, ...). I'm not 100% on the name or the params and am open to any suggestions.

Another idea I have is to use a command registration API:
I present this as an extra option in addition to or in place of other options, in case you are interested.
My portraits have additional options shown as buttons when right-clicking the portrait. I could add an API function for registering commands of your own to the portrait commands. In this case, registering "Open Tokenizer" or just "Tokenizer" as an option that appears to the user. Like any of my command registration API functions (example), this one would feature optional callbacks for when someone executes the command and whether it should be enabled. While they are rendered as buttons right now, they may eventually become context menu options, and a command registration API would ensure that the registered content is data-driven so that I can seamlessly change the button to a context menu option without you having to make any changes on your side.


In any case, I have created an inconvenience because of going off the beaten path in Foundry sheets development, but I would like to collaborate to create intentional compatibility between us.

I hope to hear your thoughts. Thanks for reading.

@MrPrimate
Copy link
Owner

I'm happy with either of the approaches - the functions that the sheet wrappers call are already exposed via an API, so there is always the fallback of that if either of the other two don't work. I would probably lean towards the api registration - that would work well, and lends itself well to having multiple tools able to bind themselves to the portrait etc

@kgar
Copy link
Contributor Author

kgar commented Jan 14, 2024

At last, I will be able to start working on this.
I will provide hooks/API for both options. Please feel free to use either or both approaches as you see fit.

Thank you for working with me on this!

@kgar
Copy link
Contributor Author

kgar commented Jan 14, 2024

Here is a near-final draft of portrait menu command registration:

Hooks.once("tidy5e-sheet.ready", (api) => {
  api.actorPortrait.registerMenuCommands([
    {
      label: "Test",
      iconClass: "fa-solid fa-flask",
      tooltip: "Click for test result",
      enabled: (params) => params.actor.type !== "vehicle",
      execute: (params) => {
        console.log(params);
        ui.notifications.info(
          "Hello, Test Portrait Menu Command for " + params.actor.name
        );
      },
    },
  ]);
});

image

image

image

@kgar
Copy link
Contributor Author

kgar commented Jan 15, 2024

Here is the hook in action:

Hooks.on('tidy5e-sheet.preOpenActorPortraitFilePicker', (actor, event) => {
  // Do something else
  return false; // prevent the portrait file picker from opening
});

This is set up to trigger for all actors when the user left-clicks / taps on the actor portrait.

I'm packaging up the release for this and will post it here when ready.

@kgar
Copy link
Contributor Author

kgar commented Jan 15, 2024

Tidy v0.2.9 has been released with the new API function and the hook described above.

@MrPrimate
Copy link
Owner

On the registerMenuCommands execute params can you include the token data like the sheet render hook does if opened from a scene? This allows me to update the tokens on the current scene with the new token image, which is often where people are opening character sheets from.

@kgar
Copy link
Contributor Author

kgar commented Jan 18, 2024

What is the prop path on that usually? Is it like data.options.token?

I can provide the entire actor sheet context a la params.context, so it would be params.context.options.token for the prop path I described above.

@kgar
Copy link
Contributor Author

kgar commented Jan 18, 2024

Update: I have released v0.2.11 with the actor sheet context prop added to the registerMenuCommands execute params field.

https://kgar.github.io/foundry-vtt-tidy-5e-sheets/interfaces/PortraitContextMenuCommandExecuteParams.html#context

@MrPrimate
Copy link
Owner

Thank you, and out in v4.2.12

@kgar
Copy link
Contributor Author

kgar commented Jan 20, 2024

Thank you again for this!

@kgar
Copy link
Contributor Author

kgar commented Jan 20, 2024

Ah, one last thing: I realized I did not update the tidy5e-sheet.preOpenActorPortraitFilePicker with the context field, which prevents you from taking the original approach of handling clicking / shift+clicking the portrait.

I am updating that hook call to include the full context.

So, if a user posts an issue requesting the original left-click behavior on the portrait to open Tokenizer, the pre hook will include the context as of my next release in a day or two:

image

This is just in case.

And thanks again! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants