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

feat(mini-message): #791 add mini-message audience #848

Draft
wants to merge 2 commits into
base: main/4
Choose a base branch
from

Conversation

AV3RG
Copy link

@AV3RG AV3RG commented Dec 10, 2022

Adds shorthand methods to send messages with mini-message strings in them easily. The current approach utilizes the delegation provided by ForwardingAudience and allows users to use a custom MiniMessage instance for each audience instead of just the default one.

TODO:

  • Shorthand method for chat messages/action bars/title parts/tab list header and footer
  • Unit tests for MiniMessageAudience

Closes #791

@zml2008
Copy link
Member

zml2008 commented Dec 11, 2022

I'm not entirely sure how I feel about this yet so I'm hoping for feedback from more people.

I do think MiniMessageAudience should be an interface that directly extends Audience and calls through to the Audience methods. A default implementation could exist private to the package, so it's only exposed via MiniMessageAudience.miniMessageAudience(Audience delegate) or something like that, rather than exposing the whole concrete class.

@AV3RG
Copy link
Author

AV3RG commented Dec 11, 2022

I do think MiniMessageAudience should be an interface that directly extends Audience and calls through to the Audience methods. A default implementation could exist private to the package, so it's only exposed via MiniMessageAudience.miniMessageAudience(Audience delegate) or something like that, rather than exposing the whole concrete class.

I agree that MiniMessageAudience should be an interface and should not directly expose the whole class but I do still think that it should extend ForwardingAudience instead of Audience. Forwarding audience provides easy delegation for the methods that are meant to be implemented by platforms with just a single audiences() method.

@kezz
Copy link
Member

kezz commented Dec 11, 2022

I agree - it should be an interface that implements Audience, not ForwardingAudience. Platforms with a player-like object shouldn't be forced to handle the overhead of all forwarding audience calls for no reason.

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

Overall, I like the idea behind this. I can see it being a useful way to introduce people to MM/adventure for platforms.

However, I do worry about the actual use of this in practice. For example, Paper couldn't really use this as sendMessage(String) conflicts. I guess they could just do a check if it contains legacy and then forward it on but I wonder if we should have these methods being sendMiniMessage, sendMiniMessageBook, etc.

Would be interested in hearing any thoughts anyone else has.

* @return a new MiniMessageAudience
* @since 4.13.0
*/
static @NotNull MiniMessageAudience audience(final @NotNull Audience audience) {
Copy link
Member

Choose a reason for hiding this comment

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

should be miniMessageAudience so it's nice for static imports

* @return a new MiniMessageAudience
* @since 4.13.0
*/
static @NotNull MiniMessageAudience audience(final @NotNull Audience audience, final @NotNull MiniMessage mini) {
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
static @NotNull MiniMessageAudience audience(final @NotNull Audience audience, final @NotNull MiniMessage mini) {
static @NotNull MiniMessageAudience audience(final @NotNull Audience audience, final @NotNull MiniMessage miniMessage) {

Comment on lines +47 to +48
this.delegate = delegate;
this.mini = mini;
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
this.delegate = delegate;
this.mini = mini;
this.delegate = Objects.requireNotNull(delegate, "delegate");
this.mini = Objects.requireNotNull(mini, "mini");

final class MiniMessageAudienceImpl implements MiniMessageAudience {

private final @NotNull Audience delegate;
private final @NotNull MiniMessage mini;
Copy link
Member

Choose a reason for hiding this comment

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

should be be miniMessage

@imDaniX
Copy link

imDaniX commented May 3, 2023

Maybe extend the idea a little for more general usage? An audience that is created with a serializer and audience instances, and then uses the serializer to convert and send string messages to the audience. Dunno about the name tho, maybe something like FancyAudience or ConvertingAudience.
Then make MiniMessageAudience extend said interface with all its TagResolvers thingies.

It might be worth to include such audience creation into the Audience interface, something like Audience#audience(ComponentSerializer<Component, Component, String> componentSerializer), but it's rather questionable.

@kezz
Copy link
Member

kezz commented May 3, 2023

Maybe extend the idea a little for more general usage? An audience that is created with a serializer and audience instances, and then uses the serializer to convert and send string messages to the audience.

Hmmm, I'm not really sure I like this idea. I think it is potentially too generic to be useful. I envision this MiniMessageAudience being a wrapper, with methods like sendMiniMessageBook, etc. I'm not sure what we'd even call a generic, String-parameterized sendMessage alternative. Plus, the docs would have to be very specific too...

Overall, when you can just sendMessage(thing.deserialize("hi")) I'm not sure how much utility this has. I can see it being useful for MiniMessage strings, considering how common they seem to be used anyway, but yeah... Overall not sure.

@imDaniX
Copy link

imDaniX commented May 3, 2023

Hmmm, I'm not really sure I like this idea. I think it is potentially too generic to be useful. I envision this MiniMessageAudience being a wrapper, with methods like sendMiniMessageBook, etc. I'm not sure what we'd even call a generic, String-parameterized sendMessage alternative. Plus, the docs would have to be very specific too...

Well, I dunno why I've mentioned only String. The concept surely can be extended by using java generics. Something ConvertingAudience<T>, where's the T stands for the type that's being converted from, and sending methods would look like sendConvertedMessage(T message).

Overall, when you can just sendMessage(thing.deserialize("hi")) I'm not sure how much utility this has. I can see it being useful for MiniMessage strings, considering how common they seem to be used anyway, but yeah... Overall not sure.

For me, MiniMessage just isn't the serializer I prefer at all, and I don't think I'm alone. It feels it would be a bit too selective to add such audience only for MiniMessage.
One can say "If they need it - they'll do it themselves", and I guess I agree. But, well, wrappers are always a pain in the arse, so if anyone would like to use something aside of MiniMessage for their project, such API would be rather helpful and will save quite a bit of time.

@AV3RG
Copy link
Author

AV3RG commented May 17, 2023

However, I do worry about the actual use of this in practice. For example, Paper couldn't really use this as sendMessage(String) conflicts. I guess they could just do a check if it contains legacy and then forward it on but I wonder if we should have these methods being sendMiniMessage, sendMiniMessageBook, etc.

I am on phone, so sorry for any incorrect quotes. Just had my last semester exams and I will get right back on this.

Yea I agree with this, I think its a simple approach that will fit well with any audience implementation.

@AV3RG
Copy link
Author

AV3RG commented May 17, 2023

Hmmm, I'm not really sure I like this idea. I think it is potentially too generic to be useful. I envision this MiniMessageAudience being a wrapper, with methods like sendMiniMessageBook, etc. I'm not sure what we'd even call a generic, String-parameterized sendMessage alternative. Plus, the docs would have to be very specific too...

Well, I dunno why I've mentioned only String. The concept surely can be extended by using java generics. Something ConvertingAudience<T>, where's the T stands for the type that's being converted from, and sending methods would look like sendConvertedMessage(T message).

Overall, when you can just sendMessage(thing.deserialize("hi")) I'm not sure how much utility this has. I can see it being useful for MiniMessage strings, considering how common they seem to be used anyway, but yeah... Overall not sure.

For me, MiniMessage just isn't the serializer I prefer at all, and I don't think I'm alone. It feels it would be a bit too selective to add such audience only for MiniMessage.
One can say "If they need it - they'll do it themselves", and I guess I agree. But, well, wrappers are always a pain in the arse, so if anyone would like to use something aside of MiniMessage for their project, such API would be rather helpful and will save quite a bit of time.

I have another idea. How about we promote the existing methods or make new methods that accept an interface called something like ComponentSerializable or Sendable something that has a serializeToComponent() method that returns a Component. That will allow you to inherit that in your classes/data types and send them directly. Ofc this approach wont work with standard java classes like String but I can see that being helpful for a lot of other stuff.

Would love to hear opinions on this.

@zml2008
Copy link
Member

zml2008 commented May 17, 2023

That already exists with ComponentLike.

I think making this interface more generic (in supporting non-MM serializers) would result in loss of functionality without benefits that outweigh that cost -- things like custom tag resolvers are nice to have in-line.

@AV3RG
Copy link
Author

AV3RG commented May 17, 2023

That already exists with ComponentLike.

I think making this interface more generic (in supporting non-MM serializers) would result in loss of functionality without benefits that outweigh that cost -- things like custom tag resolvers are nice to have in-line.

I did not know that class :P

I am not saying replace MiniMessageAudience. I am saying keep it like it is and add the ComponentLike functionality alongside in the Audience class

But then if ComponentLike already exists, I am not too sure of changing/adding new methods is worth it when you can just add a function call and then send it

@AV3RG
Copy link
Author

AV3RG commented May 30, 2023

Does anyone have any opinions on what the unit tests should check?
Kinda lost on that

@Leguan16
Copy link

Does anyone have any opinions on what the unit tests should check?
Kinda lost on that

I can take a look at it on the weekend if you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MiniMessageAudience that provides shorthand methods to send messages/etc with MiniMessage strings
5 participants