Glowkit-Legacy icon indicating copy to clipboard operation
Glowkit-Legacy copied to clipboard

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

Open Ribesg opened this issue 10 years ago • 23 comments

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 avatar Sep 10 '14 19:09 Ribesg

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

ST-DDT avatar Sep 10 '14 19:09 ST-DDT

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.

SpaceManiac avatar Sep 10 '14 20:09 SpaceManiac

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)

ST-DDT avatar Sep 10 '14 20:09 ST-DDT

Not sure, it may be worth investigating.

SpaceManiac avatar Sep 10 '14 20:09 SpaceManiac

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.

bendem avatar Sep 10 '14 20:09 bendem

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.

ST-DDT avatar Sep 10 '14 20:09 ST-DDT

Can't be sent from Server? Problem solved.

Ribesg avatar Sep 10 '14 21:09 Ribesg

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?

ST-DDT avatar Sep 11 '14 16:09 ST-DDT

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

Ribesg avatar Sep 11 '14 16:09 Ribesg

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 avatar Sep 11 '14 18:09 ST-DDT

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.

ST-DDT avatar Sep 14 '14 21:09 ST-DDT

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

dequis avatar Sep 19 '14 06:09 dequis

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.

SpaceManiac avatar Sep 19 '14 06:09 SpaceManiac

@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

Ribesg avatar Sep 19 '14 07:09 Ribesg

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

ST-DDT avatar Sep 19 '14 18:09 ST-DDT

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.

SpaceManiac avatar Sep 19 '14 19:09 SpaceManiac

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 in PlayerChatEvent as senderDetails

ST-DDT avatar Sep 19 '14 22:09 ST-DDT

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

turt2live avatar Oct 20 '14 03:10 turt2live

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

Ribesg avatar Nov 03 '14 08:11 Ribesg

These names sound right to me https://github.com/SpongePowered/SpongeAPI/pull/47#issuecomment-61437228

bendem avatar Nov 03 '14 08:11 bendem

@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

ST-DDT avatar Nov 06 '14 18:11 ST-DDT

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

bendem avatar Nov 06 '14 19:11 bendem

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)

satoshinm avatar May 27 '17 18:05 satoshinm