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

Fixed removing EChests from Tuner and Tuner opening in other GUIs #90

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

Conversation

Sefiraat
Copy link
Member

@Sefiraat Sefiraat commented Jul 5, 2021

Description

Placed Ender Boxes can be pulled out from the tuner GUI without any effort.
Found while testing this issue, the Ender Tiner will overide interactions in other GUI's (like /sf open_guide)

Changes

Fixed the return when tuningblock != null in EnderTuner#onShiftClickExtract.
Wrapped onInteractEvent with an action check.

Related Issues

N/A

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values

@Sefiraat Sefiraat requested a review from a team as a code owner July 5, 2021 17:29
Copy link
Member

@TheBusyBiscuit TheBusyBiscuit left a comment

Choose a reason for hiding this comment

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

I don't quite understand the issue nor the fix here.
Can you please explain this, perhaps with a video and some code comments?

@Sefiraat
Copy link
Member Author

Sefiraat commented Jul 6, 2021

@@ -81,7 +81,7 @@ public boolean hasGlow() {

@Override
public void onInteractItem(PlayerInteractEvent event) {
if (event.getAction() == Action.RIGHT_CLICK_AIR || event.getAction() == Action.RIGHT_CLICK_BLOCK) {
if (event.getAction() == Action.RIGHT_CLICK_AIR || event.getAction() == Action.RIGHT_CLICK_BLOCK) { // Make sure user is interacting intentionally (i.e. Not in a GUI)
Copy link
Member

Choose a reason for hiding this comment

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

  1. Code and comments should never go on the same line
  2. The comment is a bit misleading, the PlayerInteractEvent is not called for GUI events anyway?

Copy link
Member Author

@Sefiraat Sefiraat Jul 7, 2021

Choose a reason for hiding this comment

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

Given all other onInteractItem methods from other items have a getAction check before without comment and you can see the issue it's resolving, shall I just remove the comment? I originally shipped without one as it seemed to just bring it in line with other items?

As for why this fires, im not sure if its just the SF Guide, but any GUI icon clicked makes the player interact with their item in hand - so this isn't specific to the endertuner on further inspection. Here is a clip from a server with SF only :
https://user-images.githubusercontent.com/20646323/124713615-b1224180-def8-11eb-9922-e15a7c5c306b.mp4

I had made the assumption that this behavior was normal and intended, if not, let me know and ill create an issue on the SF GitHub.

Regardless, we still only want the tuner to trigger if the player is using it in a natural manner so limiting to these action types is still wise I think?

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.

2 participants