Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add invert-ignored-list config #420

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JoaoVictor834
Copy link

Add a config for invert the ignored list #419
I don't test or build this yet.

Copy link
Collaborator

@Tomut0 Tomut0 left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

@Tomut0 Tomut0 requested a review from RoinujNosde February 10, 2024 06:44
@JoaoVictor834 JoaoVictor834 requested a review from Tomut0 February 11, 2024 17:44
Comment on lines 88 to 92
public boolean isIgnored(ProtectionManager.Action action, Block block) {
return is(WAR_LISTENERS_INVERT_IGNORED_LIST) ? !getIgnoredList(action).contains(block.getType().name())
: getIgnoredList(action).contains(block.getType().name());
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!
Now everything looks good, unless one small moment.

Perhaps, you noticed that war and land features are the part of one mechanism,
ignored-list will affect on both. Consequently, it means inverted-ignore-list will prevent clan members to share their lands.

To avoid this situation, I can suggest to make ignored-list separated and each action could have it's own invert option. Something like:

war-and-protection:
  listeners:
    ignored-lists:
      war:
          PLACE:
            invert: true
            blocks:
            - "PLAYER_HEAD"
      land:
          BREAK:
            invert: false
            blocks:
            - "PLAYER_HEAD"

Then it will require to migrate old config format to a new one.
You can look at the example of migration here.

If you have other ideas, or if you have problems with the implementation, feel free to ask about it.

* @return true if the action is allowed, false otherwise.
* @see Action
*/
public boolean can(@NotNull Action action, @NotNull Location location,
Copy link

Choose a reason for hiding this comment

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

Method can has 5 arguments (exceeds 4 allowed). Consider refactoring.

@@ -213,10 +228,12 @@ private void clearWars() {
}
}

private boolean isSameClanAndAllowed(Action action, UUID owner, Player involved, String landId) {
if (!settingsManager.is(LAND_SHARING)) {
private boolean isSameClanAndAllowed(Action action, UUID owner, Player involved, String landId, @Nullable Block block) {
Copy link

Choose a reason for hiding this comment

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

Method isSameClanAndAllowed has 5 arguments (exceeds 4 allowed). Consider refactoring.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants