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

feat: Issue #425 - Add default "Link Actor Data" for Characters #445

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Stew-rt
Copy link
Contributor

@Stew-rt Stew-rt commented May 27, 2023

Add default "Link Actor Data" to new Actor's prototypeToken if it's type is character and is not from a Compendium. Also provided two extra options which would be sane but do not cover the Issue, so commented out.

Add default "Link Actor Data" to new Actor's prototypeToken if it's type is character and is not from a Compendium.
Also provided two extra options which would be sane but do not cover the Issue, so commented out.
@wyrmisis
Copy link
Collaborator

Any chance you could add some testing for this?

@Stew-rt
Copy link
Contributor Author

Stew-rt commented May 27, 2023

Sure. Just working on #424 at the mo.. so might be a little bit.

@Stew-rt
Copy link
Contributor Author

Stew-rt commented May 27, 2023

I've added tests.. but i'm not sure how this project runs them, sorry if i'm being dumb.

@Stew-rt Stew-rt changed the title feat: Issue #425 - Add default "Like Actor Data" for Characters feat: Issue #425 - Add default "Link Actor Data" for Characters May 27, 2023
@wyrmisis
Copy link
Collaborator

Not a problem! The Foundry ecosystem is just weird :)

You'll want to pick up the Quench module. That'll run tests for you.

@Stew-rt
Copy link
Contributor Author

Stew-rt commented May 29, 2023

Finally got tests for the 3 features working.
Now I know to not trust my IDE's linter at all for Foundry stuff.

Also, I had to have my test clean some stuff up after, as Quench doesn't seem to remove Compendiums created during a test routine.

Stew-rt added a commit to Stew-rt/ose that referenced this pull request May 29, 2023
sets FormApplication option, submitOnClose to false.
commented out function override, close() as it is undesirable.
Stew-rt added a commit to Stew-rt/ose that referenced this pull request May 29, 2023
* sets FormApplication option, submitOnClose to false.
* migrated Gather scores to _onSubmit override
* Added default to data.roll.type check in helpers-dice.js, setting isSuccess AND isFailure to false.
* set data object in call to OseDice.Roll to not provide type Object.
Copy link
Member

@anthonyronda anthonyronda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I ran into some issues with the current changeset

Current code changes the state of a character actor from unlinked to linked when it is copied from a world to a compendium. I would call this unexpected behavior. I believe you could rather check if there is a source.id flag before changing the setting, because if there is, it means the user likely wants the token "as-is" without a mutation

There's an additional issue: when duplicating a token, sometimes* the copy is mutated to have Link Actor Token enabled. I believe a token copy should be an exact copy, and that this is also unexpected behavior

  • Note: for some odd reason the sourceId is not the token being duplicated. The sourceId is the same as the duplicated token. This means that sometimes it doesn't provide what I think is unexpected behavior, if the original token was copied from a compendium. I'll raise this with Foundry, because I think a token copy should have its sourceId flag set to the duplicated token. I don't yet know another way to check if a token is a duplicate

@Stew-rt
Copy link
Contributor Author

Stew-rt commented May 29, 2023

Ironically, this implementation is robbed almost verbatim from the official DnD5 system, as my familiarity with the foundry base is limited.

But fair do's, I'll take another look around and try again. 🙂

@anthonyronda
Copy link
Member

5e? Who plays that? 😆

Great, thanks, no rush obviously!

@anthonyronda
Copy link
Member

anthonyronda commented Jun 9, 2023

Just leaving this here for us to monitor foundryvtt/foundryvtt#9567

@anthonyronda
Copy link
Member

anthonyronda commented Nov 15, 2023

Noting that the bugged behavior blocking this issue is still awaiting V12 (unless someone finds a better solution, but likely the linked issue above will lead to the best way of solving it)

@anthonyronda
Copy link
Member

Noting that the bugged behavior blocking this issue is still awaiting V12 (it has been pushed several times now)

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

Successfully merging this pull request may close these issues.

3 participants