rayzr522 / JSONMessage

A modern version of Fanciful
MIT License
30 stars 16 forks source link

Add replace option and JSONMessage#from #11

Open Andre601 opened 4 years ago

Andre601 commented 4 years ago

It would be nice to have a JSONMessage#replace(String, String/Object) method witch would replace any appearance of the target String with the provided String/Object (Whichever goes better).

Additionally would a JSONMessage#from(String) sure be useful too. This would essentially attempt to parse the provided String into a JSONMessage object (Essentially parsing JSON to a JSONMessage object when possible).

I currently try to implement JSONMessage into a plugin, but this requires me to replace placeholders, which I can't all just replace before the JSONMessage#then(String) method, so I have to use an own ReflectionHelper class to send the message as BungeeCord's ComponentSerializer seems to be utter trash.

Perhaps another alternative would be a String field for the JSONMessage#send(Player...) method? Like JSONMessage#send(String, Player...) which would take the String and send it to the players.

rayzr522 commented 4 years ago

@Andre601 just to clarify, i want to make sure i understand what you're trying to do... so actually three new methods are being proposed:

JSONMessage#replace(String, String) - is this one beneficial at all? can just be achieved with String#replace on the string you input into the JSONMessage

JSONMessage#replace(String, JSONMessage) - is this meant so you could, for example, replace %player% with a player's name, but also give it its own styling & make it hoverable/clickable or something along those lines?

JSONMessage#fromJson(String) - I am using the name #fromJson rather than #from because i believe this is your intention -- to be able to load a serialized JSONMessage back into JSONMessage? what's the usecase for this one? being able to have configurable JSONMessages or something / read JSONMessages from a config?

Andre601 commented 4 years ago

JSONMessage#replace(String, String) - is this one beneficial at all? can just be achieved with String#replace on the string you input into the JSONMessage

I just mentioned it in my message that I can't just do a String#replace every time I add a new JSON part, since some require stuff such as the player which at the time of creating the JSONMessage isn't available.

JSONMessage#replace(String, JSONMessage) - is this meant so you could, for example, replace %player% with a player's name, but also give it its own styling & make it hoverable/clickable or something along those lines?

I never suggested this type of method. I suggeated a static send method that would accept a JSONMessage (or perhaps also a String) to send.

JSONMessage#fromJson(String) - I am using the name #fromJson rather than #from because i believe this is your intention -- to be able to load a serialized JSONMessage back into JSONMessage? what's the usecase for this one? being able to have configurable JSONMessages or something / read JSONMessages from a config?

Yeah kind of. In my case do I have to serialize the JSON into a String, to then replace specific placeholders. And since I currently can't just generate a JSONMessage from a String is the only alternative to send the String using my own ReflectionHelper instance.

I-Al-Istannen commented 4 years ago

I just mentioned it in my message that I can't just do a String#replace every time I add a new JSON part, since some require stuff such as the player which at the time of creating the JSONMessage isn't available.

Why? Why not make a JSONMessage per player and send it to only that one player? There seems to be some context I am missing, maybe a usage example would help clear it up?

I suggeated a static send method that would accept a JSONMessage (or perhaps also a String) to send.

I didn't quite understand what "also" means here - what would that String do? Is that a replacement for:

JsonMessage.fromMinecraftJson(String).send(player)

?

In my case do I have to serialize the JSON into a String, to then replace specific placeholders.

Why can you only replace them in the generated JSON?

rayzr522 commented 4 years ago

I just mentioned it in my message that I can't just do a String#replace every time I add a new JSON part, since some require stuff such as the player which at the time of creating the JSONMessage isn't available.

one other thing to add to this is that what you're suggesting here is inherently almost impossible due to the fact that all the JSONMessage methods mutate your instance, meaning you couldn't just create one instance and use it for each player, only replacing the placeholders for each player before sending. the first time you call JSONMessage#replace, your base message would be mutated and no longer usable for other players.

i think the issue here lies in your usage pattern of JSONMessage, rather than in JSONMessage's capabilities. however, doing what you requested with replacing placeholders in JSONMessage itself would definitely be possible -- it just wouldn't solve the problem you're trying to solve (which again, is a problem in your usage pattern)

for what it's worth, there is almost no difference in performance between creating JSONMessage objects for each player (to handle player-specific placeholders) vs creating one JSONMessage and sending it to a list of players. there's a difference, but not one that matters for 99.98% of use-cases.

the rest @I-Al-Istannen has covered quite nicely so i will not reiterate his points :smile:

Andre601 commented 4 years ago

I have it setup, where I pre-load/build the JSON instances, since I see 0 benefit in always making it yet again each time a player sends a message. And since the information is (mostly) the sender of the message is a single replacement here often the easiest solution for me.

It would be (at least) useful to have the send alternative, where you could provide a JSON String. That would allow me to easly get the JSON String, replace any placeholders within it and then send it to all players, without the annoyance of having my own ReflectionHelper class just for that.

Also, I use JSONMessage in my plugin HexChat to provide JSON support: https://github.com/Andre601/HexChat

rayzr522 commented 4 years ago

I have it setup, where I pre-load/build the JSON instances, since I see 0 benefit in always making it yet again each time a player sends a message.

this isn't about whether it's beneficial or not, but rather whether it's necessary or not -- and in this case, it is. your usage pattern is fundamentally contrary to how JSONMessage works, because each message is mutable and therefore any customization per-player MUST require a separate JSONMessage object to be created for each player. (yes, if JSONMessage had a proper .clone() method, you could still kinda do what you're trying to do even with JSONMessages being mutable, but it doesn't)

the send alternative is redundant if you can parse JSON strings into a JSONMessage object, and then just send from there. send is definitely not the place to add complexity to.

i fully support the ability to serialize & deserialize JSONMessage objects, I can see a lot of potential benefits for that, but your given use case is not one of them but is rather a hack to get around the fact that there is no proper .clone() on JSONMessage and you don't wish to create a separate instance for each player.

optimally, I think moving to an immutable system in some kind of v2 rewrite would be nice, although it's too drastic of a rewrite for this current version. i think that the loading from JSON strings is a beneficial feature, as is being able to replace placeholders with custom JSONMessage objects (so that placeholders could be easily styled / have tooltips / etc).

but particularly for your use case, the best option would be to simply implement a proper JSONMessage#clone() method, as well as replace methods. that way you can construct your "base" JSONMessage, and then for each player just .clone() it and then .replace all the placeholders, then send it to that player (then rinse and repeat for each player you wish to send this to)