Pointers

[kashike]

No description provided.

[Draycia]

LGTM, would be something I use.
Display names and UUIDs are also something I commonly have attached to my Audience implementations anyway, so those default keys make sense to me.
Assuming those keys will be implemented on Player/ServerPlayer this also cuts back on some boilerplate.

[kashike]

It was suggested to me by @lucko when I first opened this pull request, to add the following - thoughts?

  AudienceKey<HasPermission> HAS_PERMISSION = key(HasPermission.class, Key.key("adventure", "uuid"));

  interface HasPermission {
    TriState hasPermission(final String permission);
  }

[Draycia]

It was suggested to me by @lucko when I first opened this pull request, to add the following - thoughts?

  AudienceKey<HasPermission> HAS_PERMISSION = key(HasPermission.class, Key.key("adventure", "uuid"));

  interface HasPermission {
    TriState hasPermission(final String permission);
  }

I can't say for certain if I think it belongs in Audience as a default key, but I would make use of it.
Any abstraction over common Player/ServerPlayer methods is less work on my end.

Edit: my primary usage of Audiences/ForwardingAudiences is in platform abstracted Player objects, so the 3 proposed keys are all ones I'd use.

[kezz]

Could this implement Keyed?

[lucko]

Some other keys that would be useful (for my uses):

AudienceKey<String> NAME - we have display name already, but this would also be very useful.
AudienceKey<AudienceType> TYPE - would require a new enum (or enum-like) class, but a way to distinguish between an audience that is a player vs a console vs a entity.... only needs to cover the basic types, or alternatively be Key based

Permission hook would be v. useful too, but I think PermissionFunction for the interface name, PERMISSIONS for the field name, and hasPermission returning a boolean would be better. Or tbh just use Predicate<String>...

[kezz]

I think it might make more sense if the actual keys (NAME, DISPLAY_NAME, etc) live in the AudienceKey interface - might be a bit messy to have the in Audience where they'll turn up all over the place, probably cleaner to keep them in one position.

Also, I love the idea of the permissions test thing - alongside some sort of enum to work out "what" the audience is, it would be nice to have some tests to work out "where" the audience is (world, server, etc).

[dualspiral]

I get what this is about - you want to reduce the amount of platform specific stuff people have to do to, say, determine whether an Audience is a player, or what its display name is, but I'm concerned how much this is beginning to really creep in on the responsibility and design of the host platform API. I'm not against what you're doing here, but, speaking from a Sponge perspective, when placed on top of our interfaces, it's mixing nullable and Optional return types and it will start looking mismatched on the various platforms. I accept on other platforms, if you were to go optional the opposite would happen.

I definately acknowledge that this is a problem you're going to have on some platforms, taking Sponge and Paper as two platforms they have vastly different philosphies. In fact, you have a key like system from Sponge which will probably look somewhat out of place on Paper, but a nullable return type that will look out of place on Sponge with our optional centric design. Other platforms may vary and I leave it to them to comment futher from their perspective.

---

A comment on the ForwardingAudience and get. There are possibly some cases where some keys may be applicable - the display name or proposed type might be used on a world, for example. However, is that useful? I'm simply going to say, no. So you've ended up with the situation where an Audience is given keys but a specific sub interface is not - and now you have to check for it.

Perhaps have an AdventureKeyHolder interface that may be a key holder, and have that separate from Audience? If you're likely going to have to check for a ForwardingAudience to avoid it, you might as well just introduce such an interface and check for that instead. Indeed, by doing so, a platform could implement Audience in the API, implement AdventureKeyHolder in the impl if they don't really want to expose this, and Adventure users could just typecheck/cast to AdventureKeyHolder to get the Adventure specific methods that an API might not otherwise want to expose for reasons above. I realise that some people here are going to baulk at that idea but please remember that some platforms just may not want this in this way in their API and thus prefer to push developers using their platform alone to use their methods.

Certainly don't throw an exception on it though if you absolutely must do it this way. That's just a bad idea because you don't want an UnsupportedOperationException bringing down the operation itself because a ForwardingAudience was used (but not checked for). If a ForwardingAudience will never return a value, then these methods should never be there. The pitfalls of inheritance, huh?

---

I know that some of what I say might not be popular, I truly do (I've certainly been at the receiving end of it before, indeed, had my comments twisted and warped over it), but Adventure needs to be careful to not upset the various platforms by imposing methods etc. that are inconsistent with the the way their hosts work. Having a more modular approach where cross platform developers can typecheck feels like it might be a solution here (plus it makes a lot more sense to pull out a "key holder" from its audience, especially if such an approach may get used elsewhere later).

I am not against what you're trying to do here, not at all, I just think that a bit of discussion and thought from the platforms and how they implement this will be warranted.

[dualspiral]

Oh, I was asked to comment on something else as well:

Some other keys that would be useful (for my uses):

AudienceKey NAME - we have display name already, but this would also be very useful.
AudienceKey TYPE - would require a new enum (or enum-like) class, but a way to distinguish between an audience that is a player vs a console vs a entity.... only needs to cover the basic types, or alternatively be Key based

Permission hook would be v. useful too, but I think PermissionFunction for the interface name, PERMISSIONS for the field name, and hasPermission returning a boolean would be better. Or tbh just use Predicate...

TYPE sounds fine but perhaps just limit it to what would be useful, CONSOLE, PLAYER and maybe RCON sound like the only three that are really of interest, right?

Permissions hooks... well that feels like that's splitting out from keys itself as I'd expect a key to return some data, not a function. I'd argue that if something like that really belongs in Adventure (and I am not convinced it does as permissions are a platform specific thing in the first place, Sponge implicit vs Paper explicit inheritance comes to mind, but other impls might not use dot separated permissions) then that needs to be a separate interface altogether.

[lucko]

I think it might help if I clarify the reasoning for all of this... (at least from my perspective)

@kashike's vision right at the start of the adventure design process was that people would be able to use Audience in place of CommandSender/Source/whatever instances in multi-platform plugins. I was (quite vocally) against having everything implement a highly generic Audience interface across the board for other reasons, but this was cited as the #\1 reason why it had to be this way... ok fine, however...

At the moment, Audience doesn't actually provide enough functionality / information to fulfil this purpose in the majority of cases.

It is very hard to implement a generic "user interface" (be that a command handler, or whatever) using just Audience because there is no way to find out information about the audience's identity, permission status, etc.

Individual plugins are left to do this abstraction themselves (see here for an example from spark) - yet the whole idea was that this wouldn't be necessary.

It's a shame that Sponge / other platforms might not be on board with this idea - but as far as I'm concerned this was always the plan from the start... and I assumed also the reason why multiple platforms would want to all adopt the same library.

[kashike]

@lucko is correct.

Here are some initial thoughts of mine:

speaking from a Sponge perspective, when placed on top of our interfaces, it's mixing nullable and Optional return types and it will start looking mismatched on the various platforms. I accept on other platforms, if you were to go optional the opposite would happen.

I originally had this system using an Optional before I published the PR, but decided on the overlords that accept defaultValue instead.

Perhaps have an AdventureKeyHolder interface that may be a key holder, and have that separate from Audience?

The problem here lies with what @lucko mentioned - having people casting for very common operations would become annoying, fast, for people who write cross-platform plugins (WorldEdit, LuckPerms, etc).

TYPE sounds fine but perhaps just limit it to what would be useful, CONSOLE, PLAYER and maybe RCON sound like the only three that are really of interest, right?

We would probably go with it being a Key based type, to not limit the choices to ones we define here.

[Draycia]

TYPE sounds fine but perhaps just limit it to what would be useful, CONSOLE, PLAYER and maybe RCON sound like the only three that are really of interest, right?

I'd prefer a more generic REMOTE over RCON, since Discord bridges are becoming increasingly more common now.
Key based would probably be better, so one could specify discord over something generic.

The problem here lies with what @lucko mentioned - having people casting for very common operations would become annoying, fast, for people who write cross-platform plugins (WorldEdit, LuckPerms, etc).

Individual plugins are left to do this abstraction themselves (see here for an example from spark) - yet the whole idea was that this wouldn't be necessary.

The design of the feature as is already says the value may or may not be present, having to check the presence of the feature on top of the values it may provide is just extra work for little gain from my perspective.

the display name or proposed type might be used on a world, for example. However, is that useful?

Then don't have worlds return a value for that key.
Any given Audience implementation is not required to provide values for every single key.
If it doesn't make sense or provide much quality to support any given key for an Audience, then simply don't do so.

That being said it may be better to move the default keys into AudienceKey, to better convey this.

it's mixing nullable and Optional return types and it will start looking mismatched on the various platforms.

If each design decision is going to look weird on some platform, then it may not be the best idea to dwell on the fact for too long.
Designing things in a way that's friendly to platforms is a good idea, but there's no pleasing everyone.
This repo is not under the SpongePowered or PaperMC orgs, I don't suspect many plugin devs expect its design decisions to match entirely with the platforms that willingly choose to adopt it.

[kashike]

Standard keys have been extracted to an AudienceKeys class.

[dualspiral]

@kashike's vision right at the start of the adventure design process was that people would be able to use Audience in place of CommandSender/Source/whatever instances in multi-platform plugins.

I suspect what you're after is a Audience & Subject & Identifiable interface in Sponge terms, something I'm actually surprised we haven't done yet - but that's not really part of this issue.

Let me be clear, the goal here is something I can support, I just don't think this is the right design choice for the reasons I've given above. The problem is the API side of it for the simple reason of the platforms who are using this library in good faith have significant and somewhat irreconcilable differences in style and function. Adventure is in the tricky situation where it's trying to satisfy every platform but it's trying to do so via a monolithic interface that the native platforms must implement alongside their "native" methods. My point is not "we don't support the goal", it's one of what it looks like within those platforms.

In Sponge, what will happen on the Player interface (as it stands) is, as a snippet:

// Adventure Audience
@Nullable T get(AudienceKey<T> key);

// Sponge ValueContainer
Optional<T> get(Key<? extends Value<T>> key);

// Sponge ValueContainer
@Nullable T getOrNull(Key<? extends Value<T>> key);

Now, I'm not saying that Adventure should not use get, in fact I have said quite the opposite, it makes sense to do so - but now you have overloads that look weird and in the Sponge case may cause some confusion from users - especialy those that aren't particularly versed in Adventure vs Sponge. However, even if Adventure used get and getOrNull, I'd still have my reservations, but only because other platforms may now look out of place and this is the sort of thing that we all just need to cast our eyes over.

This is what I'm mindful of.

---

That being said it may be better to move the default keys into AudienceKey, to better convey this.

I agree.

The design of the feature as is already says the value may or may not be present, having to check the presence of the feature on top of the values it may provide is just extra work for little gain from my perspective.

I knew it wasn't going to be popular.

This repo is not under the SpongePowered or PaperMC orgs, I don't suspect many plugin devs expect its design decisions to match entirely with the platforms that willingly choose to adopt it.

I think we need to be clear here - we choose to adopt Adventure, but that doesn't mean Adventure should be free to do what it likes because there are limits to what the platforms are going to accept. We accept the library on the terms that were apparent to us at the time, and not anything different. The relationship between Adventure and platforms is not the same as a standard library - for a start, @kashike is on most of, if not all of, the platform teams that are using it - the relationship here is not the same as a standard library. Hell, @kashike specifically asked me to comment on this PR - probably because I suspect they knew I was going to be more critical than others (yes, I'm that guy).

For what it's worth, I'm somewhat accepting of what's going to happen here - it's not the end of the world complaining over an optional and as I've said countless times, Sponge (the platform I have an interest in) is not the only platform. You're right, Adventure is not under the control of other platforms but there is an element of trust and good faith here that has to be there. I see this as something that platforms just need to look at and nod their head, voice concerns, whatever - we should not blindly accept changes but scrutinise and challenge because we are going to defend how we see things fit in our own platforms. That is what I'm doing here.

I'm not against this change. I'm critical of it, but not against it.

[Draycia]

doesn't mean Adventure should be free to do what it likes

My point there wasn't that "adventure should do what it pleases", just that if any design it chooses in this scenario leads to some platform being unhappy, it's not the healthiest to obsess about the design in that context.
Minimising design conflicts is a good goal, but eliminating it in its entirety in this scenario is unlikely.

[Electroid]

I think we should be careful not to change Audience into an identity. Audience is a nebulous group of receivers, it should not expose any identifiers.

If we want to expose more information about players, I think the way to go is with Identity. Though, I would not use a Key system. While it's simple for maintainers, it's not simple for users of the API; and we should bias for solutions that benefit users over maintainers.

We could then expose APIs that allow customizing a Component for a particular Identity.

All in all, I think the Audience interface should stay action-based (e.g. send this, show this) and then either expand Identity or introduce another interface for state-based information (e.g. who is this? what is their locale? what is their name?)

[dualspiral]

Take this back a step. If you were to do this again for Adventure 5, with the ability to break everything, how would you do it?

[kashike]

Take this back a step. If you were to do this again for Adventure 5, with the ability to break everything, how would you do it?

I've thought about this for the past day, and I keep coming back to what was in this PR prior to my latest commit as the best solution. The only other things that I have thought of, would be cross-platform incompatible (due to return types).

[dualspiral]

The only other things that I have thought of, would be cross-platform incompatible (due to return types)

I think this is what you need to do then. I don't see any other way that supports this any better given where we are in the way people want it (something else came to mind which I'll talk about below, but I suspect it's only maginally better than the other casting suggestions). It's clear the casting solution doesn't have support - and let's face it, who can blame anyone for that stance?

---

On @Electroid's point about not using keys - @kashike's point this is the ultimate problem with Adventure when considering APIs and why a key system is actually likely the best option. Adventure doesn't control the platforms, to an extent, Adventure must be considerate of the platforms, it needs to be out of the way as much as possible for the host platforms can do what they want to do. A key system minimises its footprint on other methods in a way that won't affect other methods on the target platform, but I do accept it's a break from how people that use Paper use methods. It does afford flexibility though - Adventure can just provide new keys when it wants to and the host platform won't care - it'll be totally outside of the purview of the host platform.

Now, I concur that in theory these keys are better suited to the Identifiable interface because these keys are all associated with an identifiable object, but as others have pointed out, this becomes inconvenient for consumers who then need to typecheck and cast anyway. There is no clean way to do everything this PR wants to do - so we have to make do.

With that in mind, the only other way I see this working is by having a Optional<Identifiable> asIdentifiable() (or @Nullable, though that potentially turns a one-liner into an if check and such and Java hasn't caught up with null safe operators yet) that lets you check to see if the object is an identifiable and then stick the key methods on there. This is similar to something I think @ItsDoot said with a static helper, but I don't think any platform would begrudge that - it would mean repurposing the Identifiable interface slightly but I don't really see that as a problem. But that might be an Adventure 5 thing.

Either way, rock and a hard place and something that may very well have a better solution in Adventure 5. For Adventure 4, the hand that has been dealt is limited, let's make the best of what we can.

---

We could then expose APIs that allow customizing a Component for a particular Identity.

I just want to comment here - careful about the scope of Adventure here and how other platforms might want to interact with this. Sponge, for example, has plans to be able to make the display name different based on who is viewing a player via Contextual Data. Now I can't say if this will ever come to pass, but such a method would directly clash with such a system. Adventure could use this internally, but Adventure must be careful to NOT tread on the toes of the platforms, for fear of inconsistencies occurring.

[BomBardyGamer]

Perhaps a better name for this would be PermissionHolder rather than HasPermission?

Edit: My backside is this outdated

[lucko]

A plain ol' Predicate<String> would be better.