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

Fix MOVEXY does not change owner #1527

Merged
merged 11 commits into from
Jan 19, 2023
Merged

Fix MOVEXY does not change owner #1527

merged 11 commits into from
Jan 19, 2023

Conversation

96LawDawg
Copy link
Collaborator

@96LawDawg 96LawDawg commented Jan 14, 2023

This fix works because MOVEXY will never go to a holder so no need to worry about any of those calls. I think.

Question I often find myself asking now. Does this need a file updater? Theoretically maybe, but MOVEXY has been rarely used (only in 3 PL games) and never from a hand. I guess there could be a game out there that used this is some way.

Proposed wiki update:

Parameters:

  • from: holderID (or an array) - specifies the holder that contains the widgets to move
  • collection: collection - not yet supported
  • count: number - limits the amount of moved widgets (defaults to 1 if omitted; use 0 to indicate no limit)
  • x: number - the x position on the surface (defaults to 0)
  • y: number - the y position on the surface (defaults to 0)
  • resetOwner: true/false - specifies whether the owner property of the widget is reset to null (defaults to true)

@96LawDawg 96LawDawg added bug Something isn't working routine operations changes to button behavior labels Jan 14, 2023
@96LawDawg 96LawDawg linked an issue Jan 14, 2023 that may be closed by this pull request
@ArnoldSmith86
Copy link
Owner

PR-SERVER-BOT: You can play around with it here: https://test.virtualtabletop.io/PR-1527/pr-test (or any other room on that server)

After merging, a backup will be available at https://beta.virtualtabletop.io/editor/PR1527-pr-test.

@Ingegneus
Copy link
Collaborator

I actually use that in citadels 😅 or at least it used to, but I changed it because the owner wasn't updated. the use case was to MOVEXY the entire hand to the floor, so someone else can look at it in a peek holder

@96LawDawg
Copy link
Collaborator Author

I was thinking of the other way around, where it works as bugged on production. But you gave me an idea: you could have a big chooser. All the cards dealt out on the screen in a big MOVEXY grid that only you can see. That means I need to work on a file updater.

@ArnoldSmith86
Copy link
Owner

I changed it to only remove owner if the source holder had childrenPerOwner. Please test this.

@ArnoldSmith86
Copy link
Owner

I was thinking of the other way around, where it works as bugged on production. But you gave me an idea: you could have a big chooser. All the cards dealt out on the screen in a big MOVEXY grid that only you can see. That means I need to work on a file updater.

Can only make a file updater if the behavior is optional though. Unless it GETs the owner before a MOVEXY and the SETs it again afterwards.

@96LawDawg
Copy link
Collaborator Author

Can only make a file updater if the behavior is optional though. Unless it GETs the owner before a MOVEXY and the SETs it again afterwards.

Yeah, that's the idea. Change the default behavior the way you fixed it (which I will test). Then if anybody used it this broken way, it will GET, MOVEXY, and SET.

@ArnoldSmith86
Copy link
Owner

There's no way for the file updater to know though. So this would mean wrapping every MOVEXY with SET and GET. Feels cleaner to add a resetOwner: false to every MOVEXY.

@96LawDawg
Copy link
Collaborator Author

96LawDawg commented Jan 14, 2023

There's no way for the file updater to know though. So this would mean wrapping every MOVEXY with SET and GET. Feels cleaner to add a resetOwner: false to every MOVEXY.

Yes, messy. There aren't that many MOVEXY's (cool that the new library set-up allows even me to search every PL game, but not every game on the server of course).

I don't understand what you mean. Add a new parameter?

@96LawDawg
Copy link
Collaborator Author

96LawDawg commented Jan 14, 2023

I think the latest commits do this the way @ArnoldSmith86 suggested, with a file updater. I've tested the parameter and the file importer and everything seems to work. If this is acceptable, I will also want to change the tutorial. Proposed wiki update in the first comment.

Copy link
Owner

@ArnoldSmith86 ArnoldSmith86 left a comment

Choose a reason for hiding this comment

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

Code looks fine to me. I didn't test anything though.

@Ingegneus
Copy link
Collaborator

I'll just copy LawDawg here: approved through test

@96LawDawg
Copy link
Collaborator Author

Well, I screwed up something in resolving the conflict with v10 and v11. Any ideas?

@ArnoldSmith86
Copy link
Owner

Looks like the v10 function is missing its final }. Also there's a new newline out of nowhere at the start of the file.

@96LawDawg 96LawDawg merged commit b37e6ae into main Jan 19, 2023
@96LawDawg 96LawDawg deleted the fix-ownerMOVEXY branch January 19, 2023 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working routine operations changes to button behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MOVEXY doesn't update owner
3 participants