Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Context-aware Tag #949

Closed
ejm opened this issue Jul 20, 2023 · 2 comments · Fixed by #1051
Closed

Context-aware Tag #949

ejm opened this issue Jul 20, 2023 · 2 comments · Fixed by #1051

Comments

@ejm
Copy link

ejm commented Jul 20, 2023

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
Copy link
Member

kezz commented Jul 20, 2023

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
Copy link
Author

ejm commented Jul 21, 2023

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

@kezz kezz self-assigned this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants