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 some bugs and added QoL features #181

Open
wants to merge 8 commits into
base: dev/1.4.0
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,7 @@ public String toString(@Nullable Event e, boolean debug) {
}
}
MissingReports marked this conversation as resolved.
Show resolved Hide resolved

public Expression<Object> getSlots() {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this used anywhere? If not, it should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

I think I accidently auto generated this part

return slots;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package io.github.apickledwalrus.skriptgui.elements.sections;

import ch.njol.skript.Skript;
import ch.njol.skript.config.SectionNode;
import ch.njol.skript.doc.Description;
import ch.njol.skript.doc.Examples;
import ch.njol.skript.doc.Name;
import ch.njol.skript.doc.Since;
import ch.njol.skript.lang.Expression;
import ch.njol.skript.lang.Section;
import ch.njol.skript.lang.SkriptParser.ParseResult;
import ch.njol.skript.lang.Trigger;
import ch.njol.skript.lang.TriggerItem;
import ch.njol.skript.variables.Variables;
import ch.njol.util.Kleenean;
import io.github.apickledwalrus.skriptgui.SkriptGUI;
import io.github.apickledwalrus.skriptgui.gui.GUI;
import org.bukkit.event.Event;
import org.bukkit.event.inventory.InventoryClickEvent;
import org.eclipse.jdt.annotation.Nullable;

import java.util.List;

@Name("GUI Slot Change")
@Description("Sections that will run when a gui slot is changed. This section is optional.")
@Examples({
"create a gui with virtual chest inventory with 3 rows named \"My GUI\"",
"\trun when slot 12 changes:",
"\t\tsend \"You changed slot 12!\" to player",
"\trun on slot 14 changed:",
"\t\tcancel event"
})
@Since("1.3")
public class SecSlotChange extends Section {

static {
Skript.registerSection(SecSlotChange.class,
"run when [gui] slot[s] %integers% change[s]",
"run when [gui] slot[s] %integers% [(are|is)] [being] changed",
"run on change of [gui] slot[s] %integers%"
);
}

@SuppressWarnings("NotNullFieldNotInitialized")
private Trigger trigger;
private Expression<Integer> guiSlots;

@SuppressWarnings("unchecked")
@Override
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult, SectionNode sectionNode, List<TriggerItem> triggerItems) {
if (!getParser().isCurrentSection(SecCreateGUI.class)) {
Skript.error("You can't listen for changes of a slot outside of a GUI creation or editing section.");
return false;
}

trigger = loadCode(sectionNode, "inventory click", InventoryClickEvent.class);

guiSlots = (Expression<Integer>) exprs[0];
return true;
}

@Override
@Nullable
public TriggerItem walk(Event e) {
GUI gui = SkriptGUI.getGUIManager().getGUI(e);
Integer[] slots = guiSlots.getAll(e);

for (Integer slot : slots) {
if (gui != null && slot != null && slot >= 0 && slot + 1 <= gui.getInventory().getSize()) {
Copy link
Owner

Choose a reason for hiding this comment

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

the gui != null check should be moved up:

if (gui == null)
	return walk(e, false);

additionally, slot won't ever be null

Object variables = Variables.copyLocalVariables(e);
GUI.SlotData slotData = gui.getSlotData(gui.convert(slot));
if (variables != null && slotData != null) {
slotData.setRunOnChange(event -> {
Variables.setLocalVariables(event, variables);
trigger.execute(event);
});
} else if (slotData != null) {
slotData.setRunOnChange(trigger::execute);
}
}
}

// We don't want to execute this section
return walk(e, false);
}

@Override
public String toString(@Nullable Event e, boolean debug) {
return "run on change of slot " + guiSlots.toString(e, debug);
}

}
57 changes: 55 additions & 2 deletions src/main/java/io/github/apickledwalrus/skriptgui/gui/GUI.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ public void onClick(InventoryClickEvent e) {
// Only cancel if this slot can't be removed AND all items aren't removable
e.setCancelled(!isRemovable(slotData));

// Call onChange if the slot is being changed
if (!e.isCancelled() && (e.getCursor() != null || e.getCurrentItem() != null)) {
if (e.getCursor() == null || e.getCurrentItem() == null ||
!e.getCursor().isSimilar(e.getCurrentItem()) ||
e.getCurrentItem().getAmount() < e.getCurrentItem().getMaxStackSize()) {
onChange(e);
}
}

Consumer<InventoryClickEvent> runOnClick = slotData.getRunOnClick();
if (runOnClick != null) {
SkriptGUI.getGUIManager().setGUI(e, GUI.this);
Expand All @@ -48,6 +57,28 @@ public void onClick(InventoryClickEvent e) {
}
}

@Override
public void onChange(InventoryClickEvent e) {
if (isPaused() || isPaused((Player) e.getWhoClicked())) {
e.setCancelled(true); // Just in case
return;
}

SlotData slotData = getSlotData(convert(e.getSlot()));
if (slotData != null) {
// Only cancel if this slot can't be removed AND all items aren't removable
e.setCancelled(!isRemovable(slotData));

Consumer<InventoryClickEvent> runOnChange = slotData.getRunOnChange();
if (runOnChange != null) {
Copy link
Owner

Choose a reason for hiding this comment

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

You shouldn't run the consumer if the event is cancelled.

SkriptGUI.getGUIManager().setGUI(e, GUI.this);
runOnChange.accept(e);
}
} else { // If there is no slot data, cancel if this GUI doesn't have stealable items
e.setCancelled(!isRemovable());
}
}

@Override
public void onDrag(InventoryDragEvent e) {
if (isPaused() || isPaused((Player) e.getWhoClicked())) {
Expand All @@ -61,6 +92,7 @@ public void onDrag(InventoryDragEvent e) {
break;
}
}
onChange(e);
}

@Override
Expand Down Expand Up @@ -233,6 +265,16 @@ public Character convert(Object slot) {
return nextSlot();
}

public InventoryClickEvent setClickedSlot(InventoryClickEvent event, int slot) {
Copy link
Owner

Choose a reason for hiding this comment

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

This would be better off as a static method in GUIEvents

return new InventoryClickEvent(
event.getView(),
event.getSlotType(),
slot,
event.getClick(),
event.getAction()
);
}

/**
* @return The next available slot in this GUI.
*/
Expand Down Expand Up @@ -477,7 +519,7 @@ public String getID() {
*/
public void setID(@Nullable String id) {
this.id = id;
if (id == null && inventory.getViewers().size() == 0) {
if (id == null && inventory.getViewers().isEmpty()) {
SkriptGUI.getGUIManager().unregister(this);
clear();
}
Expand All @@ -500,6 +542,8 @@ public static final class SlotData {

@Nullable
private Consumer<InventoryClickEvent> runOnClick;
@Nullable
private Consumer<InventoryClickEvent> runOnChange;
private boolean removable;

public SlotData(@Nullable Consumer<InventoryClickEvent> runOnClick, boolean removable) {
Expand All @@ -515,6 +559,11 @@ public Consumer<InventoryClickEvent> getRunOnClick() {
return runOnClick;
}

@Nullable
public Consumer<InventoryClickEvent> getRunOnChange() {
return runOnChange;
}

/**
* Updates the consumer to run when a slot with this data is clicked. A null value may be used to remove the consumer.
* @param runOnClick The consumer to run when a slot with this data is clicked.
Expand All @@ -523,12 +572,16 @@ public void setRunOnClick(@Nullable Consumer<InventoryClickEvent> runOnClick) {
this.runOnClick = runOnClick;
}

public void setRunOnChange(@Nullable Consumer<InventoryClickEvent> runOnChange) {
this.runOnChange = runOnChange;
}

/**
* @return Whether this item can be removed from its slot, regardless of {@link GUI#isRemovable()}.
* Please note that if {@link #getRunOnClick()} returns a non-null value, this method will <b>always</b> return false.
*/
public boolean isRemovable() {
return runOnClick == null && removable;
return removable;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
package io.github.apickledwalrus.skriptgui.gui;

import org.bukkit.entity.Player;
import org.bukkit.event.inventory.InventoryClickEvent;
import org.bukkit.event.inventory.InventoryCloseEvent;
import org.bukkit.event.inventory.InventoryDragEvent;
import org.bukkit.event.inventory.InventoryOpenEvent;
import org.bukkit.event.inventory.*;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -68,8 +65,21 @@ public boolean isPaused(Player player) {
}

public abstract void onClick(InventoryClickEvent e);
public abstract void onChange(InventoryClickEvent e);
public abstract void onDrag(InventoryDragEvent e);
public abstract void onOpen(InventoryOpenEvent e);
public abstract void onClose(InventoryCloseEvent e);

public void onChange(InventoryDragEvent e) {
for (int slot : e.getInventorySlots()) {
InventoryClickEvent clickEvent = new InventoryClickEvent(
e.getView(),
e.getView().getSlotType(slot),
slot,
ClickType.UNKNOWN,
InventoryAction.UNKNOWN
);
onChange(clickEvent);
}
}
APickledWalrus marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import io.github.apickledwalrus.skriptgui.SkriptGUI;
import io.github.apickledwalrus.skriptgui.gui.GUI;
import io.github.apickledwalrus.skriptgui.gui.GUIEventHandler;
import org.bukkit.GameMode;
import org.bukkit.Material;
import org.bukkit.event.EventHandler;
Expand Down Expand Up @@ -43,6 +44,7 @@ public void onInventoryClick(InventoryClickEvent event) {
if (gui == null) {
return;
}
GUIEventHandler eventHandler = gui.getEventHandler();

// Don't process unknown clicks for safety reasons - cancel them to prevent unwanted GUI changes
if (event.getClick() == ClickType.UNKNOWN) {
Expand All @@ -61,36 +63,37 @@ public void onInventoryClick(InventoryClickEvent event) {
if (clicked != null) {
Inventory guiInventory = gui.getInventory();

if (!guiInventory.contains(clicked.getType())) {
int firstEmpty = guiInventory.firstEmpty();
if (firstEmpty != -1 && gui.isRemovable(gui.convert(firstEmpty))) { // Safe to be moved into the GUI
return;
}
}

int size = guiInventory.getSize();

for (int slot = 0; slot < size; slot++) {
ItemStack item = guiInventory.getItem(slot);
if (item != null && item.getType() != Material.AIR && item.isSimilar(clicked)) {
InventoryClickEvent clickEvent = gui.setClickedSlot(event, slot);

if (!gui.isRemovable(gui.convert(slot))) {
if (item.getAmount() == 64) { // It wouldn't be able to combine
if (item.getAmount() >= item.getMaxStackSize()) { // It wouldn't be able to combine
continue;
}
event.setCancelled(true);

eventHandler.onChange(clickEvent);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's appropriate to call this here. The slot isn't removable.

return;
}

if (item.getAmount() + clicked.getAmount() <= 64) { // This will only modify a modifiable slot
if (item.getAmount() + clicked.getAmount() <= item.getMaxStackSize()) { // This will only modify a modifiable slot
eventHandler.onChange(clickEvent);
return;
}

}
}

int firstEmpty = guiInventory.firstEmpty();
if (firstEmpty != -1 && gui.isRemovable(gui.convert(firstEmpty))) { // Safe to be moved into the GUI
return;
if (firstEmpty != -1) { // Safe to be moved into the GUI
InventoryClickEvent clickEvent = gui.setClickedSlot(event, firstEmpty);
eventHandler.onChange(clickEvent);
Copy link
Owner

Choose a reason for hiding this comment

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

This should only be called if it is safe to move something into the slot I think

Copy link
Author

Choose a reason for hiding this comment

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

That part was causing a bug, I don't remember it tho

Copy link
Owner

Choose a reason for hiding this comment

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

Would you be able to test it again? My concern here is if the slot is locked but doesn't have anything in it.

Copy link
Author

Choose a reason for hiding this comment

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

Alright

Copy link
Owner

@APickledWalrus APickledWalrus Aug 26, 2024

Choose a reason for hiding this comment

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

I would recommend removing this (per our discussion above) and let me know if it gives you any issues (include code if applicable please 🙂)

Copy link
Author

Choose a reason for hiding this comment

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

I would recommend removing this (per our discussion above) and let me know if it gives you any issues (include code if applicable please 🙂)

If this part is removed, shift clicking an item from your inventory into an empty slot in the gui won't trigger the on change

if (gui.isRemovable(gui.convert(firstEmpty)))
return;
}

}
Expand All @@ -103,18 +106,42 @@ public void onInventoryClick(InventoryClickEvent event) {
// If that item is mergeable but it isn't stealable, we will cancel the event now
Inventory guiInventory = gui.getInventory();
int size = guiInventory.getSize();
ItemStack cursor = event.getWhoClicked().getItemOnCursor();
for (int slot = 0; slot < size; slot++) {
ItemStack item = guiInventory.getItem(slot);
if (item != null && item.isSimilar(cursor) && !gui.isRemovable(gui.convert(slot))) {
event.setCancelled(true);
break;
ItemStack cursor = event.getCursor();
if (cursor != null) {
Copy link
Owner

Choose a reason for hiding this comment

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

This code is starting to go off into space a little bit. It would be better to terminate exit early when possible instead. That is, do something like:

if (cursor == null)
	return;

for (int slot = 0; slot < size; slot++) {
ItemStack item = guiInventory.getItem(slot);
if (item != null && item.isSimilar(cursor)) {
if (!gui.isRemovable(gui.convert(slot))) {
event.setCancelled(true);
break;
}
if (cursor.getAmount() < cursor.getMaxStackSize()) {
InventoryClickEvent clickEvent = gui.setClickedSlot(event, slot);
eventHandler.onChange(clickEvent);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this is accurate enough. You need to keep a running total of how many items have merged into the cursor. Once you hit the max stack size the other slots (even if they have the same item) won't be modified. Same applies for the below case.

}
}
}
return;
default:
return;
}
} else {
// Call onChange if a slot is changed due to interactions within the gui itself
if (event.getClick() == ClickType.DOUBLE_CLICK) {
Inventory guiInventory = gui.getInventory();
int size = guiInventory.getSize();
ItemStack cursor = event.getCursor();
if (cursor != null && cursor.getAmount() < cursor.getMaxStackSize()) {
for (int slot = 0; slot < size; slot++) {
ItemStack item = guiInventory.getItem(slot);
if (item != null && item.isSimilar(cursor)) {
InventoryClickEvent clickEvent = gui.setClickedSlot(event, slot);
eventHandler.onChange(clickEvent);
}
}
}
}
}

gui.getEventHandler().onClick(event);
Expand Down
Loading