Glowkit-Legacy
Glowkit-Legacy copied to clipboard
Add API to use 1.7 chat features. Adds BUKKIT-5245
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 append Part
s to build a rich chat message containing (or not):
- a ChatColor compatible text, a localized text, an
ItemStack
name or anAchievement
name ; - an optional
Hover
text (showing a multi-color multi-line text, anItemStack
description or anAchievement
description) ; - an optional
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, see Message
static constructors, append
methods and insert
methods.
Message
and Part
will be ConfigurationSerializable
making it easy to save and load them from a config file.
This PR also adds methods to Player
to send a Message
and two methods to Server
and Bukkit
to broadcast a Message
. 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:
BRANCH=BUKKIT-5245-v2
mkdir ./build
cd ./build
git clone https://github.com/Ribesg/Bukkit.git -b $BRANCH
git clone https://github.com/Ribesg/CraftBukkit.git -b $BRANCH
git clone https://github.com/Ribesg/TestPlugin.git -b $BRANCH
cd ./Bukkit && mvn install
cd ../CraftBukkit && mvn install
cd ../TestPlugin && mvn install
cd ../..
mkdir ./run
mkdir ./run/plugins
cp ./build/CraftBukkit/target/craftbukkit-1.7.10-R0.1-SNAPSHOT.jar ./run
cp ./build/TestPlugin/target/TestPlugin.jar ./run/plugins
cd ./run
java -jar craftbukkit-1.7.10-R0.1-SNAPSHOT.jar
Relevant PR(s):
B-1111 - Original Bukkit PR ~~CB-1413 - Original CraftBukkit PR~~ B-1065 & ~~CB-1378~~ - Old PRs
JIRA Ticket:
BUKKIT-5245 - https://bukkit.atlassian.net/browse/BUKKIT-5245
Authors
Of the previous PRs and of this one
- @bendem did a lot of things (Mostly Bukkit part)
- @Ribesg did a lot of other things (CB part and Bukkit part)
- @ST-DDT did the Javadocs for multiple Bukkit files
Same question as usual: Anything left to do? Anybody working on Glowstone side?
https://github.com/SpongePowered/SpongeAPI/pull/15
Also lists twitch_user_info
(disabled) and open_file
as Click
Actions.
Did we ignore them consciously, did we miss them or don't they exist at all?
(That PR is not related to our version)
Just for reference: SpongeAPI version of this PR: https://github.com/SpongePowered/SpongeAPI/pull/47
open_file
definitely exists - the client uses it for the links when you take a screenshot with F2
, but I'm not sure if it's really useful. I don't know about twitch_user_info
.
Are the links limited to the screenshot folder or may it be possible to open a MC resource file with that as well? (Some kind of documentation served in a resource pack)
Not sure, it may be worth investigating.
I don't think open_file
should be implemented. It would mean possibility for plugin to open the player's files (potentially scripts and such) which would imo be a security hole.
https://github.com/SpongePowered/SpongeAPI/pull/15#issuecomment-55176938
open_file
looks like it works with anything accessible from new File(String)twitch_user_info
opens the Twitch user GUI, it's used only for Twitch chat integration.Both of them are disallowed from server chat.
Can't be sent from Server? Problem solved.
In 1.8 this messages will be used almost everywhere where text are supported.
So maybe it would be usefull to repalce chat
with text
package?
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.
The method used in ChatEvent.getFormatedMessage
could be usefull for other scenarios as well.
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. Example some kind of ingame event notification.
// 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: http://wiki.vg/Chat States that there are actually 4 types of Parts. Text, LocalizedText, Selector and Score, i guess we should replace the boolean with an enum.
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:
- [ ] Books (??)
- [ ] Signs
- [ ] Player list names
- [ ] Inventory titles
As a note, other places where Messages are used (but aren't in scope for this PR):
- Titles
- Player list header/footer
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).
See https://github.com/Ribesg/CraftBukkit/compare/37c79691615fbd28b49c7371a64700e4f5713eca...BUKKIT-5245-v2
@SpaceManiac Can Player.setDisplayName()
be converted to use Messages too?
What should be converted for Books
?
Author, Title, Pages
Only pages i guess because author and title are only shown in the item meta thing.
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: https://github.com/Ribesg/Bukkit/pull/18
We have two open points to check:
- Books
-
Player.setDisplayName()
because it may be used inPlayerChatEvent
assenderDetails
@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.
- Strange uses of
Message
- Signs
- Inventories
- Player list name
- Kick messages (prelogin and general kick/ban)
- Server list ping
- Books
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 Message
object being used for ease of coloring the applicable strings, in which case if I'm correct in saying that those things don't support components like links then it should be documented in the applicable methods (such as in setting the kick message in an event).
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.
- Components should be named "____Component" or "____Part"
- The base class,
Part
, should beChatComponent
orChatPart
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 Part
is org.bukkit.chat.Part
, isn't it? If you still think this has to be changed, it will be done.
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 https://github.com/SpongePowered/SpongeAPI/pull/47#issuecomment-61437228
@bendem Shall we rename the classes then? What about moving them to the text package instead of the chat package? This contains more features then just chat options.
Some things in Sponge hints that there is also a Click.CHANGE_PAGE
which only makes sense for books.
Click: https://github.com/SpongePowered/SpongeAPI/pull/47/files#r19930852 Hover: https://github.com/SpongePowered/SpongeAPI/pull/47/files#r19930933
Yeah, a chat package doesn't make much sense in 1.8.
I think what I read on the Sponge PR makes sense (not sure about the book only thingy - TextClickAction.Type.CHANGE_PAGE
- tho).
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 player.spigot().sendMessage(textcomponent)
, examples: https://www.spigotmc.org/threads/colors-in-config-and-clickevent-action-open_url.61753/ https://www.spigotmc.org/threads/clickevent-doesnt-work.65457/
edit: but, not yet supported in Glowstone:
Caused by: java.lang.UnsupportedOperationException: Not supported yet.
at org.bukkit.entity.Player$Spigot.sendMessage(Player.java:1734)