KyoriPowered / adventure

A user-interface library, formerly known as text, for Minecraft: Java Edition
https://docs.advntr.dev/
MIT License
682 stars 106 forks source link

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

Open AV3RG opened 1 year ago

AV3RG commented 1 year ago

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:

Closes #791

zml2008 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

imDaniX commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Leguan16 commented 11 months ago

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.