Fix and enhance EffOpenInventory #5531

TheLimeGlass · 2023-03-17T06:15:34Z

Description

Fixes EffOpenInventory to user error if a certain inventory type is used that cannot be created and opened.
Cleaned up the effect and added support for enchanting tables, loom, stonecutters, smithing tables, cartography tables and grindstones.
Made use of an enum class to allow for future implementation, includes version and method checking constructors.
Removed the deprecation of show/open to apply what it's been saying. Show = non interactable. Open = interactable. Example being a crafting table. If using the Spigot method openWorkbench, crafting recipes will be allowed, show will not include crafting recipes as it's just an inventory view. Due to this, this is a breaking change.
The past DROPPER, DISPENSER and ANVIL implementation did not actually solve the issues Anvil inventory doesn't let you do anything meaningful #41, Anvil Inventory support #3035 and Unusable Anvil GUI #4586

Target Minecraft Versions: 1.14+ for some inventory types.
Requirements: PaperSpigot for some inventory open actions.
Related Issues: #5495

AyhamAl-Ali

Well done 🚀 Amazing and clean changes

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

APickledWalrus

Good stuff. Just a few things.

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

…ript into fix/open-inventory

APickledWalrus

Looking really good so far :)
Inventory section is a nice addition

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

Efnilite · 2024-10-03T06:36:59Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+		Object object = this.inventory.getSingle(event);
+		if (object == null)
+			return super.walk(event, false);
+		if (object instanceof Inventory) {


Suggested change

if (object instanceof Inventory) {

if (object instanceof Inventory inventory) {

Efnilite · 2024-10-03T06:37:09Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+			return super.walk(event, false);
+		if (object instanceof Inventory) {
+			inventory = (Inventory) object;
+		} else if (object instanceof InventoryType) {


Suggested change

} else if (object instanceof InventoryType) {

} else if (object instanceof InventoryType inventoryType) {

Efnilite · 2024-10-03T06:38:20Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+		} else if (object instanceof InventoryType) {
+			inventory = EffOpenInventory.createInventory((InventoryType) object);
+		} else {
+			assert false;


Suggested change

assert false;

assert false;

return super.walk(event, false);

Efnilite · 2024-10-03T06:39:12Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+		if (inventory instanceof Literal) {
+			Literal<?> literal = (Literal<?>) inventory;
+			Object object = literal.getSingle();
+			if (object instanceof InventoryType && !((InventoryType) object).isCreatable()) {


Suggested change

if (object instanceof InventoryType && !((InventoryType) object).isCreatable()) {

if (object instanceof InventoryType inventoryType && !(inventoryType.isCreatable()) {

Efnilite · 2024-10-03T06:39:20Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+
+		inventory = exprs[0];
+		players = (Expression<Player>) exprs[1];
+		if (inventory instanceof Literal) {


Suggested change

if (inventory instanceof Literal) {

if (inventory instanceof Literal literal) {

Efnilite · 2024-10-03T06:41:21Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+@Name("Open/Close Inventory Section")
+@Description({
+	"Opens an inventory to a player.",
+	"The section then allows to modify the event-inventory."


Suggested change

"The section then allows to modify the event-inventory."

"This section allows easy modification of the opened inventory."

sovdeeth · 2024-10-03T19:23:49Z

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

-				"(open|show) ((0¦(crafting [table]|workbench)|1¦chest|2¦anvil|3¦hopper|4¦dropper|5¦dispenser) (view|window|inventory|)|%-inventory/inventorytype%) (to|for) %players%",
-				"close [the] inventory [view] (to|of|for) %players%", "close %players%'[s] inventory [view]");
+				"show %inventory/inventorytype% (to|for) %players%",
+				"open [a[n]] " + OpenableInventorySyntax.construct() + " [view|window|inventory] (to|for) %players%",


This means you cannot open chests, barrels, hoppers, droppers, dispensers, or a myriad of other inventories to players, only show them.

sovdeeth · 2024-10-03T19:24:19Z

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

+	@Nullable
+	private OpenableInventorySyntax syntax;


Suggested change

@Nullable

private OpenableInventorySyntax syntax;

private @Nullable OpenableInventorySyntax syntax;

sovdeeth · 2024-10-03T19:24:40Z

src/main/java/ch/njol/skript/effects/EffOpenInventory.java

+
+	@Nullable
+	private OpenableInventorySyntax syntax;
+
 	@Nullable


needs inlining

sovdeeth · 2024-10-03T19:26:53Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+		"\topen event-inventory to all players"
+})
+@Since("INSERT VERSION")
+public class SecOpenInventory extends Section {


wouldn't it be easier to have this be an EffectSection, then EffOpenInventory can become just closing inventories and all the inventory logic is concentrated in this class? The this could support anvils and such as well.

sovdeeth · 2024-10-03T19:32:19Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+	static {
+		Skript.registerSection(SecOpenInventory.class, "[show|open|create] %inventory/inventorytype%  [to %players%]");
+	}


Needs a event-inventory to be registered. Event-player could also be nice.

sovdeeth · 2024-10-03T19:35:16Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+	}
+
+	static {
+		Skript.registerSection(SecOpenInventory.class, "[show|open|create] %inventory/inventorytype%  [to %players%]");


Suggested change

Skript.registerSection(SecOpenInventory.class, "[show|open|create] %inventory/inventorytype% [to %players%]");

Skript.registerSection(SecOpenInventory.class, "[show|open] %inventory/inventorytype% [(to|for) %players%]");

sovdeeth · 2024-10-03T19:35:32Z

src/main/java/ch/njol/skript/sections/SecOpenInventory.java

+		InventorySectionEvent inventoryEvent = new InventorySectionEvent(inventory);
+		Object localVars = Variables.copyLocalVariables(event);
+		Variables.setLocalVariables(inventoryEvent, localVars);
+		TriggerItem.walk(trigger, inventoryEvent);
+		Variables.setLocalVariables(event, Variables.copyLocalVariables(inventoryEvent));
+		Variables.removeLocals(inventoryEvent);
+		if (players != null)
+			for (Player player : players.getArray(event))
+				player.openInventory(inventory);


What do you think about not running the section at all if players.getArray is empty?

APickledWalrus

Feel free to re-request me once the requested changes by other contributors have been addressed.

Fix and open EffOpenInventory

e612dd3

TheLimeGlass added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Mar 17, 2023

Clean up description

70c0cbb

TheLimeGlass added the breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) label Mar 17, 2023

AyhamAl-Ali approved these changes Mar 17, 2023

View reviewed changes

src/main/java/ch/njol/skript/effects/EffOpenInventory.java Outdated Show resolved Hide resolved

Update src/main/java/ch/njol/skript/effects/EffOpenInventory.java

9457867

Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>

APickledWalrus requested changes Mar 17, 2023

View reviewed changes

TheLimeGlass added 2 commits March 17, 2023 13:22

Allow open %inventory%

7b5535a

Merge branch 'fix/open-inventory' of https://github.com/SkriptLang/Sk…

0379c5d

…ript into fix/open-inventory

TheLimeGlass requested a review from APickledWalrus March 17, 2023 19:25

TheLimeGlass added 2 commits March 18, 2023 02:19

Add open inventory section

88fa197

Add open inventory section

e81961e

TheLimeGlass requested a review from UnderscoreTud March 21, 2023 09:04

APickledWalrus added the feature Pull request adding a new feature. label Mar 22, 2023

APickledWalrus requested changes Mar 22, 2023

View reviewed changes

Apply suggestions from code review

0e32724

Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>

TheLimeGlass requested review from APickledWalrus and Moderocky April 25, 2023 15:23

Moderocky reviewed Jun 28, 2023

View reviewed changes

src/main/java/ch/njol/skript/effects/EffOpenInventory.java Outdated Show resolved Hide resolved

TheLimeGlass and others added 2 commits June 30, 2023 23:31

Merge branch 'master' into fix/open-inventory

6b47b1c

Apply changes

25d8f7e

TheLimeGlass requested a review from Moderocky July 1, 2023 05:38

TheLimeGlass added 4 commits June 30, 2023 23:45

Apply to both classes

b27abf7

Apply changes

8eea6de

Include IllegalArgumentException

9b73d71

Remove un-needed imports

f1816e3

Moderocky approved these changes Jul 1, 2023

View reviewed changes

TheLimeGlass mentioned this pull request Aug 27, 2023

Fix an issue with creating a non creatable inventory from ExprNamed and fix array store exception #5943

Closed

Moderocky force-pushed the master branch from bd134d0 to 3f08853 Compare September 16, 2023 15:17

Moderocky force-pushed the master branch 2 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59

Moderocky changed the base branch from master to dev/feature September 18, 2023 10:22

Merge branch 'dev/feature' into fix/open-inventory

28526ad

sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023

sovdeeth requested changes Dec 30, 2023

View reviewed changes

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Show resolved Hide resolved

sovdeeth removed the 2.8 Targeting a 2.8.X version release label Dec 31, 2023

TheLimeGlass commented Jan 1, 2024

View reviewed changes

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

TheLimeGlass added 2 commits December 31, 2023 18:39

Update src/main/java/ch/njol/skript/sections/SecOpenInventory.java

75a8a30

Update src/main/java/ch/njol/skript/sections/SecOpenInventory.java

3d19268

TheLimeGlass requested a review from sovdeeth January 1, 2024 01:40

Merge branch 'dev/feature' into fix/open-inventory

c2666e2

TheLimeGlass changed the base branch from dev/feature to dev/patch February 1, 2024 20:31

Base automatically changed from dev/patch to master June 1, 2024 19:38

sovdeeth changed the base branch from master to dev/feature September 13, 2024 14:44

Merge branch 'dev/feature' into fix/open-inventory

20f2191

sovdeeth requested changes Sep 13, 2024

View reviewed changes

Apply suggestions from code review

87c4407

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

TheLimeGlass commented Oct 3, 2024

View reviewed changes

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

Update src/main/java/ch/njol/skript/sections/SecOpenInventory.java

8e1f813

TheLimeGlass requested a review from sovdeeth October 3, 2024 06:02

Merge branch 'dev/feature' into fix/open-inventory

9082c82

TheLimeGlass commented Oct 3, 2024

View reviewed changes

src/main/java/ch/njol/skript/sections/SecOpenInventory.java Outdated Show resolved Hide resolved

Update src/main/java/ch/njol/skript/sections/SecOpenInventory.java

7326af1

Efnilite requested changes Oct 3, 2024

View reviewed changes

sovdeeth requested changes Oct 3, 2024

View reviewed changes

APickledWalrus reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and enhance EffOpenInventory #5531

Fix and enhance EffOpenInventory #5531

TheLimeGlass commented Mar 17, 2023 •

edited by AyhamAl-Ali

Loading

AyhamAl-Ali left a comment

APickledWalrus left a comment

APickledWalrus left a comment

Efnilite Oct 3, 2024

Efnilite Oct 3, 2024

Efnilite Oct 3, 2024

Efnilite Oct 3, 2024

Efnilite Oct 3, 2024

Efnilite Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

sovdeeth Oct 3, 2024

APickledWalrus left a comment

	if (object instanceof Inventory) {
	if (object instanceof Inventory inventory) {

	} else if (object instanceof InventoryType) {
	} else if (object instanceof InventoryType inventoryType) {

	if (object instanceof InventoryType && !((InventoryType) object).isCreatable()) {
	if (object instanceof InventoryType inventoryType && !(inventoryType.isCreatable()) {

	if (inventory instanceof Literal) {
	if (inventory instanceof Literal literal) {

	"The section then allows to modify the event-inventory."
	"This section allows easy modification of the opened inventory."

	Skript.registerSection(SecOpenInventory.class, "[show\|open\|create] %inventory/inventorytype% [to %players%]");
	Skript.registerSection(SecOpenInventory.class, "[show\|open] %inventory/inventorytype% [(to\|for) %players%]");

Fix and enhance EffOpenInventory #5531

Are you sure you want to change the base?

Fix and enhance EffOpenInventory #5531

Conversation

TheLimeGlass commented Mar 17, 2023 • edited by AyhamAl-Ali Loading

Description

AyhamAl-Ali left a comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

APickledWalrus left a comment

Choose a reason for hiding this comment

TheLimeGlass commented Mar 17, 2023 •

edited by AyhamAl-Ali

Loading