Skip to content

Improve permission handling around interaction events #10599

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

Open
wants to merge 6 commits into
base: version/main
Choose a base branch
from

Conversation

Thodor12
Copy link
Contributor

@Thodor12 Thodor12 commented Jan 15, 2025

Changes proposed in this pull request

  • Fixed several warnings from appearing when attempting to open any building with only the ACCESS_HUT permission
  • Fixed a bug that caused additional warnings to appear upon townhall opening due to the color list sending a color change event upon window open (unnecessary event send removed)
  • Split the open inventory for buildings and citizens so individual permissions may be configured for each action (also should these be OPEN_CONTAINER, ACCESS_HUT or MANAGE_HUT? Currently MANAGE_HUT (potentially related, technically accessing the inventory should be ACCESS_HUT, but modifying the inventory should be MANAGE_HUT, can we make an inventoy readonly?)
  • Improved event handling in the permission event handler
    • Right click block interactions don't chain through all possible options (previously, to open a hut block it requires ACCESS_HUT permission, if the block was a hut block, and person did have ACCESS_HUT permission, the method would continue instead of returning, causing other permissions to be checked unnecessarily, potentially throwing more warnings)
    • Trapdoors have been added to toggleabes
    • OPEN_CONTAINER doesn't check for any BaseEntityBlock, rather it checks if the tile entity implements Container
    • Right click items have been separated from right click blocks
    • Potion throwing permission checks for any item implementing ThrowablePotionItem, rather than PotionItem (so person may still drink self-consume potions)

Testing

  • Yes I tested this before submitting it.
  • I also did a multiplayer test.

Review please

someaddons
someaddons previously approved these changes Jan 26, 2025
# Conflicts:
#	src/main/java/com/minecolonies/core/client/gui/townhall/WindowMainPage.java
@Raycoms
Copy link
Contributor

Raycoms commented Jan 26, 2025

Did you test this very well for all the permission things you changed, both where you should have and shouldn't have permission? It touches a lot of stuff, so it should be well tested

@Thodor12
Copy link
Contributor Author

Thodor12 commented Jan 26, 2025

I ensured that all events under every condition work as they should, I had a MP server open where I slowly granted the other player additional permissions to see if all the conditions worked as they should.

Note: There are some odd behavioural ones, but that's unfortunate due to how Mojang calls these different events.
For example you may not place blocks if you don't have right click permission, because the entityPlacement gets called first, and upon not having right click permission, you may not actually place the block, despite having placement permissions.

@@ -52,6 +54,13 @@ public InteractionClose(
this.key = key;
}

@Override
@Nullable
public Action permissionNeeded()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't all interactions need the same condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reason I did this is because there is no reason for a message that closes a window to require any kind of permission level, on top of that the default is MANAGE_HUTS, which is wrong to begin with

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, we should just have all interactions have the same and not have a new function in all of them for some edge case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an override, this method exists in the abstract message and is set by default to MANAGE_HUTS, I haven't added anything new, just lowered the permissions required for triggering this message

Copy link
Contributor

@Raycoms Raycoms left a comment

Choose a reason for hiding this comment

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

Before merge, please do another SMP test and test some fundamental interactions with 2 colonies existing in the same world.

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