KyoriPowered / adventure

A user-interface library, formerly known as text, for Minecraft: Java Edition
https://docs.advntr.dev/
MIT License
679 stars 105 forks source link

Context-aware `Tag` #949

Closed ejm closed 3 months ago

ejm commented 11 months ago

This is a continuation of an old series of conversations in Discord

It would be useful for there to be a unified way for Tags to accept additional contextual information, such as a player or entity that is the subject of a MiniMessage. This can be useful for dynamic placeholders. As a potential example tag, using the Sponge API:

static Tag itemInHand(final ArgumentQueue args, final Context ctx) {
    Object representedObject = ctx.representedObject(); // A new method in Context that is populated in MiniMessage::deserializeWithContext
    if (representedObject instanceof ArmorEquipable) {
        ArmorEquippable holder = (ArmorEquipable) representedObject;
        ItemStack inHand = holder.itemInHand(HandTypes.MAIN_HAND); // ItemStack implements ComponentLike
        return Tag.inserting(inHand);
    }
    return Tag.inserting(Component.of("[not holding anything]"));
}

TagResolver inHandResolver = TagResolver.resolver("item_in_hand", this::itemInHand);

ServerPlayer player = ...; // This will be our "context object"
sendMessage(MiniMessage.miniMessage().deserializeWithContext("I'm holding <item_in_hand>", player, inHandResolver);

I specifically chose an Object to be the type of the "context object," though others (including the library's maintainers) have suggested using Pointered instead. I understand the rationale behind Pointered, but I worry that it would be too limiting to require every object which might be used contextually to implement Pointered. In the example above, for instance, any Sponge ArmorEquippable would also need to be Pointered and have the appropriate Pointer values.

The specific changes I see being needed to accommodate this:

  1. The addition of MiniMessage::deserializeWithContext(String, Object, TagResolver...) and similar methods
  2. The addition of Context::getRepresentedObject() or similar
  3. Documentation to support this model

Thanks!

kezz commented 11 months ago

I am still pretty solidly convinced that the context object should be Pointered, or perhaps Audience (although that is less useful). Having Object existing in a public API is so yucky and will just force everyone to instanceof all the time (which, you will likely do sometimes with Pointered too, but at least in this case you can actually use it without that sometimes). I do understand the annoyance with using a type like this though, so it could come with some expansion of the Pointered API, perhaps stuff like #429 or maybe even a way to "wrap" an object inside a pointered impl, idk.

I think the term "context" is odd. I don't like having to get the context from the context, that just feels weird to me. Not sure if I can think of a better name at the moment though.

Overall, I think this could be made generic. A ContextualSerializer perhaps (for want of a better name for context)?

ejm commented 11 months ago

Looking around a bit more, I think Pointered would be fine, so long as most platforms support it. My fear was that we would have entities (or other objects) that wanted to be represented but wouldn't be Pointered and would have to be wrapped or something along those lines, which could be pretty annoying and plugin-specific.

As for the term "context," I agree with you fully. I dislike the term, which is why I borrowed a page from [Sponge](https://jd.spongepowered.org/spongeapi/11.0.0-SNAPSHOT/org/spongepowered/api/placeholder/PlaceholderContext.html#associatedObject()) and gave it a different name within the Context class. I'm not sure what to call it otherwise.

What benefit would there be to having a generic ContextualSerializer for MiniMessage? I don't see this pattern being applicable elsewhere in Adventure. It's not really my place to say much one way or the other on it, I'm just curious where it would be used.