-
Notifications
You must be signed in to change notification settings - Fork 12
Add API to use 1.7 chat features. Adds BUKKIT-5245 #8
base: master
Are you sure you want to change the base?
Conversation
Still misses all static constructors, append and insert methods JDs Also make Part constructor private
Added some javadocs
Update Events to use the new Messages
Not sure, it may be worth investigating. |
I don't think |
SpongePowered/SpongeAPI#15 (comment)
|
Can't be sent from Server? Problem solved. |
In 1.8 this messages will be used almost everywhere where text are supported. |
Why not! I'm not yet aware of every new uses of this there are now. Books and Signs I think? Something else? |
Another thing that just appeared in my mind. Message.from(String message, Parts... parts) {
for (int x = 0;x < parts; x++) {
return message.replaceAll("%x", parts[x]);
}
} Think of situations where server owners have to configure some kind of message in the config. // from config
message="%0 joined in %1"
// static
part0=Part(player.getName(), Hover(player.getUUID()))
part1=Part(Click(SEND_TEXT, "/join " + arena.getName()), arena.getName(), Hover("Click here to join as well")) Do you think so too? |
We should change this PR once more: |
Minor improvements
Soooo... what about the craftbukkit side of this PR? Was it a significant amount of code? |
As I understand, the CB side was only a partial implementation pending a more finalized API. I plan to start on the GS-side implementation of this soon. Since we're targeting 1.8 now, would love to see:
As a note, other places where Messages are used (but aren't in scope for this PR):
As observed, I see Selector and Score in the docs. I think it would be good to determine the details of those. |
@dequis there was some fun, but not much. Everything wasn't done, just the Message -> Mojangson thing (using internals). |
@SpaceManiac Can What should be converted for The scoreboads, do they support Messages? |
I can confirm scoreboards do not support rich messages. I'm actually not entirely sure on the deal with books, will have to experiment, so save that for a little later if you want. |
Here is the PR adding the MC v1.8 features: We have two open points to check:
|
Added MC 1.8 features
@Ribesg @ST-DDT I've glanced over this PR again because I really hate open PRs (and someone mentioned it in IRC) and have a few comments on the overall PR. They may contradict what has been said previously, but that's what happens when you have time to think. I understand if either of you are missing in action when it comes to this stuff. If not though, please consider this message. In terms of quality, this PR is pretty high up there. The structure is nice and easy to read, understand, and implement in a plugin. Many of the other chat APIs failed to do this and clearly yours came on top. I personally like it and would like to use it as soon as possible as it is well thought out and overall just a really nice addition to have. Now that Glowstone is working on 1.8, this should be pulled fairly quickly. Before getting on to the code concerns, please review the below list of oddities I found while rebriefing myself of the codebase. If you can provide a justification for them, that's fine. Otherwise I ask that they be corrected.
These are strange simply because from what I recall it is not possible to add certain components, like links, to these things. If I'm incorrect on this, please correct me. I can see the Due to this being Glowkit and not Bukkit, some formatting concerns were noted (mainly the strictness of JavaDocs, additional formatting, etc) that would have been required in Bukkit. However those changes may not be required here. Hopefully @SpaceManiac can follow up on that point to see if the excessive documentation changes are required (ie: remove or keep them in the PR). Additional concerns about the PR is naming for the time being. As I mentioned earlier, I haven't reviewed it in depth for a while and may have missed some things (or even contradicted what I have said previously), but I have noticed the following while scanning the code as a reminder to myself.
As a final note, I was hoping that you, or someone else, would be able to open an accompanying Glowstone pull request so that this can be pulled with implementation as soon as possible. Without the implementation component this PR doesn't have much support and may not even be pulled. Hopefully you are able to update this PR accordingly or are willing to accept pull requests on your branch from other authors so this PR can still advance. Thank you for taking the time to read my essay. |
This PR was originally for 1.7, adding only 1.7 features. 1.8 features weren't in it at first, they were added later. I should spend some time studying the changes, I have done nothing for 1.8 myself. For the Javadoc, I don't think there's any problem improving it. I completely agree with Bukkit guidelines on that point. An API should always target perfection, nothing less. On the naming concerns, and as I said on the SpongeAPI PR, the idea was to keep short names as static methods could be used a lot on one line. In the end, the full class name of For the Glowstone part, I'm planning to do it at some point in the future if nobody does it before. I didn't have the chance to look at Glowstone's code yet, I have absolutly no knowledge here. Maybe it will be time to get some. If anybody can get me some pointers on where to start :-) |
These names sound right to me SpongePowered/SpongeAPI#47 (comment) |
* @return the built Message | ||
*/ | ||
public static Message ofLocalized(String id, String... parameters) { | ||
return Message.of(Part.of(id, parameters)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As already mentioned in the Sponge Part.
Should be Part.ofLocalized
EDIT: PR created Ribesg#19
@bendem Shall we rename the classes then? Some things in Sponge hints that there is also a Click: |
Bugfix Message
Yeah, a chat package doesn't make much sense in 1.8. |
Anyone trying to find out how to send rich messages in modern Glowstone, and has found this API pull request, the way to do it now seems to be edit: but, not yet supported in Glowstone:
|
The Issue:
There are currently no easy way to use the new chat features introduced in 1.7.
Justification for this PR:
As said before, there are currently no way (except from directly using the
/tellraw
command and passing your own generated json to it) to use the 1.7 chat features.PR Breakdown:
This PR adds a builder interface based on a
Message
to which you appendPart
s to build a rich chat message containing (or not):ItemStack
name or anAchievement
name ;Hover
text (showing a multi-color multi-line text, anItemStack
description or anAchievement
description) ;Click
action (executing commands / sending chat message, proposing commands / proposing chat message, or opening urls (actually opens the prompt)).These messages handle colors and formatting using ChatColor just like standard messages, internally using the existing algorithm built by the great @mbax.
Many (many) shortcuts have been added to
Message
to make it easier to append specific elements, seeMessage
static constructors,append
methods andinsert
methods.Message
andPart
will beConfigurationSerializable
making it easy to save and load them from a config file.This PR also adds methods to
Player
to send aMessage
and two methods toServer
andBukkit
to broadcast aMessage
. Those methods are closed to the standards String methods.Usage examples can be found in the TestPlugin after this line. You can get builds of all of this and test it yourself using the link below.
This PR is a complete rewrite based on the previous one and on @feildmaster's work on the subject.
Testing Results and Materials:
Test Plugin Source
Build everything yourself to test it as we're not allowed to link CB builds here.
Quick build & test:
Relevant PR(s):
B-1111 - Original Bukkit PR
CB-1413 - Original CraftBukkit PRB-1065 &
CB-1378- Old PRsJIRA Ticket:
BUKKIT-5245 - https://bukkit.atlassian.net/browse/BUKKIT-5245
Authors
Of the previous PRs and of this one