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

[WIP] Add kick message expr #7658

Open
wants to merge 18 commits into
base: dev/feature
Choose a base branch
from
Open

Conversation

nopeless
Copy link

@nopeless nopeless commented Mar 1, 2025

Description

Implement kick message getting and setting


Target Minecraft Versions: Not known yet

Related Issues:

https://discord.com/channels/135877399391764480/1332620961328926790

@nopeless nopeless changed the title Add kick message expr [WIP] Add kick message expr Mar 1, 2025
@Aresiel
Copy link

Aresiel commented Mar 1, 2025

OMG I literally had this issue today and this PR was made like the second I found it. What are the odds.

@nopeless
Copy link
Author

nopeless commented Mar 1, 2025

Annotations need proper skript version needs to be updated, hence the WIP

@Burbulinis
Copy link
Contributor

This PR should target dev/feature, not master

@Fusezion

This comment was marked as resolved.

@sovdeeth
Copy link
Member

sovdeeth commented Mar 1, 2025

Annotations need proper skript version needs to be updated, hence the WIP

Any additions should use INSERT VERSION as a placeholder. It's automatically replaced during release

@RequiredPlugins("xxx Paper 1.16.5+")
@Since("xxx 2.8.0")
@Events("Kick")
public class ExprKickMessage extends SimpleExpression<String> {
Copy link
Member

@sovdeeth sovdeeth Mar 1, 2025

Choose a reason for hiding this comment

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

This class needs to follow the proper code conventions re: method order, using finals in parameters, annotation placement.
But more importantly, this should probably be part of ExprMessage itself.

Copy link
Author

Choose a reason for hiding this comment

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

working on the method order;

I don't think it should be part of ExprMessage, its not really a message that appears in chat. The reason why I named it ExprKickMessage is because kick reason was already taken

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 no ExprKickReason, though?

Copy link
Author

Choose a reason for hiding this comment

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

... good point, but then the class name would be ExprKickReason while the actual pattern is kick message and I thought that was worse

Copy link
Member

Choose a reason for hiding this comment

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

Why would the pattern stay as kick message? shouldn't it be kick reason too?

@nopeless nopeless changed the base branch from master to dev/feature March 1, 2025 17:59
@nopeless
Copy link
Author

nopeless commented Mar 1, 2025

This should already be possible via ExprMessage

QUIT("quit", "(quit|leave|log[ ]out|kick)( |-)message", PlayerQuitEvent.class, PlayerKickEvent.class) {
@Override
@Nullable
String get(final Event e) {
if (e instanceof PlayerKickEvent)
return ((PlayerKickEvent) e).getLeaveMessage();
else
return ((PlayerQuitEvent) e).getQuitMessage();
}
@Override
void set(final Event e, final String message) {
if (e instanceof PlayerKickEvent)
((PlayerKickEvent) e).setLeaveMessage(message);
else
((PlayerQuitEvent) e).setQuitMessage(message);
}
},

@Fusezion
This turns out to set the message in the chat not the kick message itself, hence the change

@Fusezion
Copy link
Contributor

Fusezion commented Mar 1, 2025

@Fusezion This turns out to set the message in the chat not the kick message itself, hence the change

Hmm, alright.

@nopeless
Copy link
Author

nopeless commented Mar 1, 2025

@sovdeeth If you have a class name recommendation let me know because its very ambiguous overall

Copy link
Contributor

@TheAbsolutionism TheAbsolutionism left a comment

Choose a reason for hiding this comment

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

I know Sovde already mentioned about the coding conventions, so I won't do anything related to that.

Comment on lines +77 to +78
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}
}
}
}

@ShaneBeee
Copy link
Contributor

In my opinion, rather than creating another expression, this should probably just be merged in with:
https://docs.skriptlang.org/expressions.html?search=#ExprMessage

@nopeless
Copy link
Author

nopeless commented Mar 2, 2025

@ShaneBeee ExprMessage is what gets sent in chat, no? This is for what gets displayed on the player's screen

@ShaneBeee
Copy link
Contributor

@ShaneBeee ExprMessage is what gets sent in chat, no? This is for what gets displayed on the player's screen

ohh ok gotcha. Anyways, wont your expression collide with kick message from ExprMessage?

@nopeless
Copy link
Author

nopeless commented Mar 2, 2025

@ShaneBeee It does, currently, I'm not sure how to resolve it. My friends and people here has obviously said that all of this is ambiguous and I am leaving it up to maintainers to decide on the pattern and name

I originally wanted to name the class KickReason but after figuring out that kick reason is taken, I renamed it to KickMessage, only to realize that kick message is also taken so I hope you find something suitable

@nopeless nopeless requested a review from sovdeeth March 3, 2025 02:07
@nopeless nopeless requested a review from Efnilite March 3, 2025 13:52
public class ExprKickMessage extends SimpleExpression<String> implements EventRestrictedSyntax {

static {
Skript.registerExpression(ExprKickMessage.class, String.class, ExpressionType.SIMPLE, "kick (message|reason)");
Copy link
Member

@Efnilite Efnilite Mar 3, 2025

Choose a reason for hiding this comment

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

Suggested change
Skript.registerExpression(ExprKickMessage.class, String.class, ExpressionType.SIMPLE, "kick (message|reason)");
Skript.registerExpression(ExprKickMessage.class, String.class, ExpressionType.SIMPLE, "(display[ed]|on-screen) kick (message|reason)");

there should be a way to differentiate the chat kick message from this one

import ch.njol.skript.doc.*;
import org.jetbrains.annotations.Nullable;

@Name("Kick Message")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Name("Kick Message")
@Name("On-screen Kick Message")

@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Mar 3, 2025
@nopeless
Copy link
Author

nopeless commented Mar 3, 2025

@Efnilite I'm guessing that the expression pattern is final? (I don't know if you have discussed with others)

@Fusezion
Copy link
Contributor

Fusezion commented Mar 3, 2025

@Efnilite I'm guessing that the expression pattern is final? (I don't know if you have discussed with others)

Just providing my two cents, I would prefer going with Efy's changes here as that not only fixes conflictions but is descriptive enough that players can tell the difference between this and it's counter part

@Efnilite
Copy link
Member

Efnilite commented Mar 3, 2025

@Efnilite I'm guessing that the expression pattern is final? (I don't know if you have discussed with others)

if you have better suggestions feel free to use them, we just need to make sure its different

@nopeless
Copy link
Author

nopeless commented Mar 4, 2025

I'll leave this open for a bit just in case someone wants to pitch in. personally I don't care what the pattern is as long as its usable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants