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

Allow players to set their hats by clicking on their helmet slot #960

Closed
wants to merge 7 commits into from

Conversation

mdcfe
Copy link
Member

@mdcfe mdcfe commented Nov 6, 2016

Allow players to set their hats by clicking on their helmet slot; resolves #956

I'll clean up the extra imports in a couple of hours.

@@ -621,6 +623,14 @@ public void onInventoryClickEvent(final InventoryClickEvent event) {
if (user.isInvSee() && (!user.isAuthorized("essentials.invsee.modify") || invOwner.isAuthorized("essentials.invsee.preventmodify") || !invOwner.getBase().isOnline())) {
event.setCancelled(true);
refreshPlayer = user.getBase();
} else if (ess.getSettings().isDirectHatAllowed() && ess.getUser(event.getWhoClicked()).isAuthorized("essentials.hat") && event.getSlot() == 38 && event.getClick() == ClickType.LEFT) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd front load it with minimal-effort checks like the slot int and click type, ending in the isAuthorized call.

@@ -621,6 +623,14 @@ public void onInventoryClickEvent(final InventoryClickEvent event) {
if (user.isInvSee() && (!user.isAuthorized("essentials.invsee.modify") || invOwner.isAuthorized("essentials.invsee.preventmodify") || !invOwner.getBase().isOnline())) {
event.setCancelled(true);
refreshPlayer = user.getBase();
} else if (ess.getSettings().isDirectHatAllowed() && ess.getUser(event.getWhoClicked()).isAuthorized("essentials.hat") && event.getSlot() == 38 && event.getClick() == ClickType.LEFT) {
if (event.getCursor().getType() != Material.AIR && event.getCursor().getType().getMaxDurability() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in a separate if?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't get around to merging them. I'll restructure the if statements now.

@mbax
Copy link
Member

mbax commented Nov 6, 2016

Naming concern: Direct... hat?

What does that even mean.

@mdcfe
Copy link
Member Author

mdcfe commented Nov 6, 2016

"Direct" as in "directly" setting a hat by clicking on the inventory slot (as opposed to "indirectly" setting a hat by holding it in the primary hand and doing '/hat').

@mbax
Copy link
Member

mbax commented Nov 6, 2016

It feels confusing, because to me it implies that without this setting you are unable to click a helmet, and click it on your head to wear it.

@mdcfe
Copy link
Member Author

mdcfe commented Nov 6, 2016

@mbax That's a fair point, but at the same time, the permission essentials.hat could suggest to some that without the permission, you are unable to equip helmets.

@mbax
Copy link
Member

mbax commented Nov 6, 2016

@mdcfe
Copy link
Member Author

mdcfe commented Nov 22, 2016

@SupaHam Could you review this by any chance? (If needed, I believe you automatically have push access to the PR branch.)

@SupaHam
Copy link
Member

SupaHam commented Dec 23, 2016

@md678685 You can place items in the head slot with right clicking as well I believe.

inv.setHelmet(event.getCursor());
event.setCursor(head);
}
} else if (ess.getSettings().isDirectHatAllowed() && event.getClick() == ClickType.LEFT && event.getSlot() == 38 && event.getCursor().getType() != Material.AIR && event.getCursor().getType().getMaxDurability() == 0 && ess.getUser(event.getWhoClicked()).isAuthorized("essentials.hat")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@SupaHam Does right-clicking send a left click event? There is a check here - ... && event.getClick() == ClickType.LEFT && ...

edit: ancient review I started ages ago and never submitted

Copy link
Member

Choose a reason for hiding this comment

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

Right click will be of type ClickType#RIGHT.

Copy link
Member

Choose a reason for hiding this comment

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

There's more logic to consider because depending on what exists in the slot or on cursor, different amounts of the stack would be placed.

@SupaHam
Copy link
Member

SupaHam commented Aug 4, 2017

This has been merged via 7958cd0. You can grab the feature from build 514 on the CI server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Features and feature requests.
Projects
None yet
3 participants