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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 Add inventory raw slot expression #4593

Merged
merged 13 commits into from Aug 29, 2023

Conversation

AyhamAl-Ali
Copy link
Member

Description

This PR Adds a new expression to get the raw inventory slot number, this is a unique number for each view, it basically consider the current opened inventories (top and bottom) as a 1 inventory that begins from the top left


Target Minecraft Versions: Any
Requirements: None
Related Issues: #4461

@TPGamesNL TPGamesNL added the feature Pull request adding a new feature. label Feb 12, 2022
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

I'm concerned that this is handled as a number whereas the regular slot supports getting and setting the item - even if Bukkit doesn't handle this as easily you could try and support this for parity with the existing expression to avoid confusing users.

@AyhamAl-Ali
Copy link
Member Author

I'm concerned that this is handled as a number whereas the regular slot supports getting and setting the item - even if Bukkit doesn't handle this as easily you could try and support this for parity with the existing expression to avoid confusing users.

I think the idea of raw slot is to just get the slot number, otherwise users can use clicked slot, having both clicked slot and raw slot as a slot is redundant IMO

@APickledWalrus
Copy link
Member

There is the index of %slots% expression. Maybe this expression could be something like raw slot index to clear up any potential confusion?

Maybe that is more confusing though actually lol

@Moderocky
Copy link
Member

There is the index of %slots% expression. Maybe this expression could be something like raw slot index to clear up any potential confusion?

Maybe that is more confusing though actually lol

Should this not be raw index instead of raw slot then?

@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Feb 14, 2022

I was thinking of adding this to index of slot as Kenzie said, but thought not then, if that's a better approach then i will try it out if possible

More discussion here

@TheLimeGlass
Copy link
Collaborator

Not a fan of this addition, raw slot is basically never used.

@AyhamAl-Ali
Copy link
Member Author

Not a fan of this addition, raw slot is basically never used.

It's indeed needed in some places if not many when you open 2 inventories the usual slot number will continue after 63 in the below gui while the raw will begin from 0

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Was there a reason we didn't change this to return Slot? I forget

src/main/java/ch/njol/skript/expressions/ExprRawSlot.java Outdated Show resolved Hide resolved
@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Jul 18, 2022

Was there a reason we didn't change this to return Slot? I forget

Here is more info about our discussion

EDIT: New discussion in skript-dev

Co-authored-by: TPGamesNL <29547183+TPGamesNL@users.noreply.github.com>
@TheLimeGlass TheLimeGlass removed their request for review July 21, 2022 10:04
@TheLimeGlass
Copy link
Collaborator

I don't believe Skript should have this expression. I'm removing my review request from this pull request.

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Just the one thing.

Also, @TheLimeGlass, what are your reasons for opposing this feature? Sure, it's not widely used by any means, but it's nice to have when needed. I don't see how it could be harmful to have in any way.

src/main/java/ch/njol/skript/util/slot/SlotWithIndex.java Outdated Show resolved Hide resolved
@TheLimeGlass TheLimeGlass added the up for debate When the decision is yet to be debated on the issue in question label Apr 18, 2023
@AyhamAl-Ali AyhamAl-Ali merged commit 91a6e9f into SkriptLang:master Aug 29, 2023
4 checks passed
Moderocky pushed a commit to Moderocky/Skript that referenced this pull request Sep 16, 2023
NotSoDelayed pushed a commit to NotSoDelayed/Skript that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants