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

Improve sign interaction handling #2142

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

Jikoo
Copy link
Member

@Jikoo Jikoo commented Aug 22, 2023

  • Preserves improved UX for 1.20 vanilla behavior where sign is opened on right click
    • Denial is done immediately and UI is not opened rather than deny post-edit
  • Improves compatibility with other plugins
    • No longer blanket cancelling interaction should make it easier for sign-based shops etc. to operate
      • Does not address GP messages with items that have vanilla behaviors GP needs to block. These plugins should look to update their handling to make it clearer that the vanilla interaction will not occur.
    • Increasing listener priority gives other plugins a chance to register their listeners before GP's

While this does require us to compile against 1.20.1, it should still be compatible with 1.17 because Bukkit handles non-existent events by logging the attempt at registration and moving on. This will result in a harmless warning on startup for older versions.

Closes #2134
Closes #2085

Jikoo added 2 commits August 22, 2023 10:38
Preserve UX of not having sign UI open when GP will block the change
Drop handling that causes issues with other plugins
Should make it easier for other plugins to override GP's behavior while still running before normal plugins.
@Jikoo
Copy link
Member Author

Jikoo commented Aug 22, 2023

CC @rgaminecraft and @MacTh3Mac: Is this sufficient for your cases? Do you have suggestions for how to handle this better?

@MacTh3Mac
Copy link

MacTh3Mac commented Aug 22, 2023

CC @rgaminecraft and @MacTh3Mac: Is this sufficient for your cases? Do you have suggestions for how to handle this better?

@Jikoo I can confirm this version compiles and runs as expected. It also behaves as desired.

Ignores waxed signs interaction
Ignores signs already protected by other software (silently)
Allows our software, 3rd party plugins and essentialsx to run as expected.
Protects editable signs that are not protected by any other method to remain uneditable.

Note I have only tested this on 1.20.1 (Paper latest).

My thanks for saving me manually compiling and forking all future versions going forward!. Just need someone to merge into master for the world to enjoy. Cheers.

@rgaminecraft
Copy link

We have tested this and everything seems to be operating as we would expect it to. Thank you for looking into this and having such a quick turnaround time, much appreciated!

@Bobcat00
Copy link

Is there a build I can use to test this? The second commit isn't on AppVeyor and I can't figure out how to use git to get these two commits.

@Jikoo
Copy link
Member Author

Jikoo commented Aug 22, 2023

Only the second commit should have been built because I pushed both at once (wanted to keep the priority changes separate in case Robo disagrees with that, easier to just revert the commit), should be this build: https://ci.appveyor.com/project/RoboMWM39862/griefprevention/builds/47853375/artifacts

@Bobcat00
Copy link

I guess it works OK, but I get inconsistent behavior with some very old signs. When right-clicking on a sign, sometimes I get the GP no build permission message, sometimes nothing happens. I guess the internal data of the sign varies based on when it was created.

I always get the no build message with new, unwaxed signs.

@Jikoo
Copy link
Member Author

Jikoo commented Aug 22, 2023

Hmm. I get that it could be a bit confusing if a sign appears like it should be editable. Half wonder if GP should just not display a message when blocking interaction, but that puts us into the "why won't this danged sign open?" area, which seems like a worse state.

@Bobcat00
Copy link

Again, this is within someone else's claim, so the player should be able to figure it out.

@RoboMWM RoboMWM merged commit ee0d17b into GriefPrevention:master Oct 4, 2023
@Jikoo Jikoo deleted the dev/better-signs branch October 4, 2023 22:48
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.

chestshop and GriefPrevention bug Swap to checking if sign is waxed with 1.20 update
5 participants