Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Add API to use 1.7 chat features. Adds BUKKIT-5245 #8

Open
wants to merge 72 commits into
base: master
Choose a base branch
from

Conversation

Ribesg
Copy link

@Ribesg Ribesg commented Sep 10, 2014

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 Parts to build a rich chat message containing (or not):

  • a ChatColor compatible text, a localized text, an ItemStack name or an Achievement name ;
  • an optional Hover text (showing a multi-color multi-line text, an ItemStack description or an Achievement 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

Ribesg and others added 30 commits July 17, 2014 13:59
Still misses all static constructors, append and insert methods JDs
Also make Part constructor private
Update Events to use the new Messages
@SpaceManiac
Copy link
Member

Not sure, it may be worth investigating.

@bendem
Copy link

bendem commented Sep 10, 2014

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.

@ST-DDT
Copy link

ST-DDT commented Sep 10, 2014

SpongePowered/SpongeAPI#15 (comment)

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.

@Ribesg
Copy link
Author

Ribesg commented Sep 10, 2014

Can't be sent from Server? Problem solved.

@ST-DDT
Copy link

ST-DDT commented Sep 11, 2014

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?

@Ribesg
Copy link
Author

Ribesg commented Sep 11, 2014

Why not! I'm not yet aware of every new uses of this there are now. Books and Signs I think? Something else?

@ST-DDT
Copy link

ST-DDT commented Sep 11, 2014

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?

@ST-DDT
Copy link

ST-DDT commented Sep 14, 2014

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.

@dequis
Copy link

dequis commented Sep 19, 2014

Soooo... what about the craftbukkit side of this PR? Was it a significant amount of code?

@SpaceManiac
Copy link
Member

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.

@Ribesg
Copy link
Author

Ribesg commented Sep 19, 2014

@dequis there was some fun, but not much. Everything wasn't done, just the Message -> Mojangson thing (using internals).

See Ribesg/CraftBukkit@37c7969...BUKKIT-5245-v2

@ST-DDT
Copy link

ST-DDT commented Sep 19, 2014

@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?

@SpaceManiac
Copy link
Member

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.

@ST-DDT
Copy link

ST-DDT commented Sep 19, 2014

Here is the PR adding the MC v1.8 features:
Ribesg#18

We have two open points to check:

  • Books
  • Player.setDisplayName() because it may be used in PlayerChatEvent as senderDetails

@turt2live
Copy link

@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 be ChatComponent or ChatPart

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.

@Ribesg
Copy link
Author

Ribesg commented Nov 3, 2014

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 :-)

@bendem
Copy link

bendem commented Nov 3, 2014

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));
Copy link

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

@ST-DDT
Copy link

ST-DDT commented Nov 6, 2014

@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

@bendem
Copy link

bendem commented Nov 6, 2014

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).

@satoshinm
Copy link

satoshinm commented May 27, 2017

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)

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

Successfully merging this pull request may close these issues.

7 participants