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

added property hidePlayerCursors #1829

Merged
merged 10 commits into from
Oct 1, 2023
Merged

added property hidePlayerCursors #1829

merged 10 commits into from
Oct 1, 2023

Conversation

ArnoldSmith86
Copy link
Owner

@ArnoldSmith86 ArnoldSmith86 commented Sep 28, 2023

This is keeping it simple. It might be a good idea to highlight the widget instead of showing a cursor but this PR really just hides the cursor.

It is added to new hands by default and also to any holder with childrenPerOwner: true by using a file updater.

@ArnoldSmith86 ArnoldSmith86 added enhancement New feature or request widget properties changes to widget properties labels Sep 28, 2023
@ArnoldSmith86
Copy link
Owner Author

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

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

@96LawDawg
Copy link
Collaborator

I can't find any flaws in this. I tested every widget type. Works as expected (except for maybe decks where it doesn't work, but who cares because it works on cards). I also tested when a widget is "invisible" because of onlyVisibleForSeat and it works properly there as well (meaning the cursor is only hidden if the widget is displayed on the screen for the player occupying that seat).

The only question I have. Should we make this the default for hands?

@ArnoldSmith86
Copy link
Owner Author

Should we make this the default for hands?

Done, but only for new hands. Not sure if we should do a file updater and set it to every childrenPerOwner: true holder. Feels like a bit of overkill.

Also not sure if the PCIO importer should use this. Does anybody know if PCIO supports hiding cursors?

Copy link
Collaborator

@96LawDawg 96LawDawg left a comment

Choose a reason for hiding this comment

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

Approved through test. I think it is fine as is without changing existing hands or any childrenPerOwner stuff. Leave it at hands. As far as I know, other than making all cursors the same color to anonymize users, there are no other cursor settings.

@RaphaelAlvez
Copy link
Collaborator

RaphaelAlvez commented Sep 29, 2023

Should we make this the default for hands?

Done, but only for new hands. Not sure if we should do a file updater and set it to every childrenPerOwner: true holder. Feels like a bit of overkill.

Also not sure if the PCIO importer should use this. Does anybody know if PCIO supports hiding cursors?

this shouldn't break any game. It is just an extra feature. I feel this falls under that conversation we had on Discord.
This is one of the biggest complaints for hands and I think we should make it the default.

ALL hands old and new. You are basically removing an annoyance.

@96LawDawg
Copy link
Collaborator

Just coming back to this. The vote on Discord, such as it was: @RaphaelAlvez and jade-alarm for retroactive application; @96LawDawg neutral. No way to tell for @ArnoldSmith86 based on how he had to set up the scoring.

Here's my point. This is ready now. Preparing a file-updater will take additional time. @mousewax already has a game ready to go. For future hands after this PR, it will be automatic anyway. Does retroactive application really matter? I don't think so. The problem of distracting cursors probably only exists in a small subset of existing games any way. And there is probably also a very small subset of games where developers would not want the cursor to go away. Why hold this up for a file-updater?

@RaphaelAlvez
Copy link
Collaborator

Does retroactive application really matter? I don't think so. The problem of distracting cursors probably only exists in a small subset of existing games any way.

I believe that almost any game that has a hand has this anoyance. Of course we get used to it but it is not a feature. On top of that if you sorta hand, it's somewhat easy to know which card is being played.

And there is probably also a very small subset of games where developers would not want the cursor to go away.

Yeah that's the whole point. I cannot think of any situation where leaving cursors on hands is better. Breaking any game is very unlikely. So why leave it?

So if we are going to be less strict about this fear of breaking games this is an excellent first step.

@ArnoldSmith86
Copy link
Owner Author

You guys do realize that I already added the file updater?

@96LawDawg
Copy link
Collaborator

You guys do realize that I already added the file updater?

Obviously not. Oops; missed that.

So I checked several games and the file updater worked on most of them. But not on Card Games and Knaves. Not that big of a deal because can always add that manually. But it is a little concerning that it missed those. Their hand holders meet the criteria for having it added. Wonder why it missed?

@ArnoldSmith86 ArnoldSmith86 enabled auto-merge (squash) October 1, 2023 20:42
@ArnoldSmith86 ArnoldSmith86 merged commit 24342d2 into main Oct 1, 2023
@ArnoldSmith86 ArnoldSmith86 deleted the feature-hideCursors branch October 1, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request widget properties changes to widget properties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants