Skip to content

Resolve issues with actor duplication#33

Open
apewall wants to merge 4 commits into
free-beer:masterfrom
apewall:duplication-fix
Open

Resolve issues with actor duplication#33
apewall wants to merge 4 commits into
free-beer:masterfrom
apewall:duplication-fix

Conversation

@apewall

@apewall apewall commented Mar 19, 2023

Copy link
Copy Markdown
Contributor

This removes relying on the functions mentioned in #31 and uses the current actor object instead. After some testing it seems to work though I'm not sure it is the best solution.

Other related issues with the character sheet that I'd also like to resolve in this pull but I need some direction of where to look to address them:

  • Character sheet tabs not being instantiated which causes tabs on all sheets that are open to change when clicked.
  • Clicking buttons on the character sheets (Damage, Repair, Break, Casting) seem to sometimes cause a javascript failure that crashes the browser window.

@apewall apewall changed the title Duplication fix to resolve #31 Resolve issues with actor duplicatoin Mar 19, 2023
@apewall apewall changed the title Resolve issues with actor duplicatoin Resolve issues with actor duplication Mar 19, 2023
@free-beer

Copy link
Copy Markdown
Owner

I'll take a look through these changes when I get a chance. With regards to the other two issues that you've mentioned, are these things you were seeing before you applied your changes? If so, it's probably better to open issues for them. In relation to the second one specifically can you provide more detail as I personally have never seen that happen?

@apewall

apewall commented Mar 20, 2023

Copy link
Copy Markdown
Contributor Author

The other problems with the sheets mentioned are not caused by these changes. I'll go ahead and open up new issues with some examples and we can discuss those separately.

@free-beer

Copy link
Copy Markdown
Owner

So I've thought about this a little more. Given this is such a substantial change I think that to incorporate it it would have to be behind a system setting that is off by default. It complicates things a little but would ensure that people could opt-in to the change.

@apewall

apewall commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

Could you summarize what you are thinking the end-user concern with this change would be? I assume most are working around the current behavior by avoiding duplicating actors entirely.

@free-beer

Copy link
Copy Markdown
Owner

Well, the reality is, no-one else has even mentioned this issue. So, if as you say, others are working around this issue, then fundamentally changing this would violate an expectation on their part. By placing this behind an opt-in option then they can knowingly and explicitly change across.

@apewall

apewall commented Mar 21, 2023

Copy link
Copy Markdown
Contributor Author

I guess I'm failing to understand in what way users would be functionally utilizing the duplication bug.

If they are not already duplicating actors they would not notice this change and remain unaffected in their workflow.

If they duplicate actors the current behavior is both non-intuitive and destructive since deleting from one actor affects a different actor with a matching itemId instead the one they are actually interacting with.

Ultimately it is up to you, but I don't see a real benefit in making this into an option.

@yriart

yriart commented Sep 19, 2023

Copy link
Copy Markdown

Hey, just commenting to say that this bugfix would be helpful for me as well. I can't really imagine how the current state would be preferable since it just makes it so duping sheets doesn't work.

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