Make ownership checks consistent between All Players and explicit assignment #4404
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `Token.isOwnedByAny(Collection<String>)` check if any provided players own the token. - `Token.isOwnedByNone()` if the token is not owned by anyone. Both of these respect `Token.isOwnedByAll()` so it doesn't have to be checked separately. These places were simplified as a result: - `AppUtil.ownedByOnePlayer()` - `EditTokenDialog.commit()` Also `TokenPropertyFunctions.getOwners()` was simplified, but that was possible without these new methods.
We had checks to ensure that lib:tokens owned by any players did not have their onCampaignLoad even executed. But the check didn't `continue` the correct loop, so outside of `Token.isOwnedByAll()`, it had no effect. This changes that to use `Token.isOwnedByAny()`, and now the right loop is naturally manipulated. Also removes some unecessary nesting of blocks and an unused method.
The existing check marked macros as trusted if the library had no listed owners, but did not consider whether it was owned by _All Players_. The updated version does that implicitly by using `Token.isOwnedByNone()`.
The existing check did not return tokens that were owned by _All Players_. The new check handles this implicitly via `Token.isOwnedByAny()`.
Also move some check out of the inner loop so we save on a bunch of unnecessary work.
cwisniew
approved these changes
Nov 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Fixes #4403
Fixes #4362
Description of the Change
These new methods are added to
Tokenfor easier ownership checks:isOwnedByAny(Collection<String>) playerIds)to check if a token has any owners out of a set of possible owners.isOwnedByNone()to check if a token has not owners.These allow replacing existing and inconsistent uses of
Token.getOwners(), ensuring that the All Players setting is respected across the board. The remaining uses ofToken.getOwners()are needed to display and manipulate the owner list, or - in the case of thegetTokens()macro function - to check very specific conditions.These bug fixes were done using the new methods:
getTokens()with an owner list will now return tokens owned by all players.onCampaignLoadis no longer run on lib:tokens owned by all players ([Bug]: Player-editable Lib:token macros are considered trusted when owned by All Players #4403).Possible Drawbacks
For
getTokens(), if any framework code is relying on excluding All Players ownership, it will need updating.Documentation Notes
N/A
Release Notes
This change is