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

[WIP] Refactor and implement ChangeDataHolderEvent.ValueChange #1695

Closed
wants to merge 7 commits into
base: bleeding
from

Conversation

Projects
None yet
5 participants
@Aaron1011
Member

Aaron1011 commented Nov 19, 2017

SpongeAPI | SpongeCommon | SpongeForge

This pull request implements ChangeDataHolderEvent.ValueChange, and extends the Data API to better support plugins using the event.

Overview

This PR makes three main sets of changes: Improvements to ChangeDataHolderEvent.ValueChange, the addition of a new event filter @GetKey, and the addition of extra utility methods to the Data API.


ChangeDataHolderEvent.ValueChange

For consistency with other events, this event now follows the getOriginalXXX, getXXX, setXXX format for its methods.

To give maximum flexibility to plugins, the previous DataTransactionResult is completely replaced when calling setChanges. Plugins wishing to preserve existing changes can use DataTransactionResult$Builder#absorbResult to combine their desired changes with previous changes.


@GetKey event filter

As demonstrated in the attached demonstration plugin, the event can be somewhat cumbersome to use. To make it easier for plugins to listen for data changes, a new event filter @GetKey is added. Using the specified key id, this event filter sets the annotated parameter to the value retrieved from either a ChangeDataHolderEvent.ValueChange, or a DataHolder.

For example, this listener:

@Listener
public void eventListener(ChangeDataHolderEvent.ValueChange event, @GetKey("FOOD_LEVEL") int foodLevel) {
    System.out.println("Food level: " + foodLevel);
 }

will be called with foodLevel set to the value associated with Keys.FOOD_LEVEL. If a value for Keys.FOOD_LEVEL is not present in the event, the listener will not be called. The parameter can be specified as the underlying value (e.g. int), a Value, or an ImmutableValue.

As described in the javadocs for @GetKey, this event filter can also be used with a DataHolder.


Data API Changes

To support the usage of this event, several changes have been made to the Data API. The most significant of these is DataHolder#offerWithEvent. This method is used by the implementation (and optionally by plugins) to fire a ChangeDataHolderEvent.ValueChange event. This method retrieves the current value from the DataHolder, constructs a DataTransactionResult containing the old and new values, and fires the ChangeDataHolderEvent.ValueChange event. If the event is not cancelled, the SUCCESS values will be actually offered to the DataHolder.

However, this method isn't sufficient to make ChangeDataHolderEvent.ValueChange easily usable for plugins. To make DataTransactionResult easier to use even without the event filter, a new enum DataCategory is added. This enum can be used with @GetKey, as well as the newly added get and getKey methods on DataTransactionResult, to lookup a value from the sucessful, rejected, or replaced set of values. (It also has the advantage of greatly simplifying the implementation of @GetKey)

getDefault and getDefaultValue

Finally, two methods are added to ValueContainer - getDefault and getDefaultValue. These methods solve the problem of creating a new value wrapper (a subtype of Value or ImmutableValue) given the underlying object.

For example, consider Keys.FOOD_LEVEL. Since this is a MutableBoundedValue, it has extra information associated with it - a minimum and a maximum value. These pieces of additional information may vary depending on the DataHolder the value came from. If a plugin wants to create a new MutableBoundedValue for a food level - if it wants to add it to a ChangeDataHolderEvent.ValueChange, for instance - it must first get access to an existing value from the proper DataHolder. Otherwise, it has no way of knowing the proper minimum and maximum values.

Unfortunately, the current Data API only allows retrieving a Value or ImmutableValue when it is already stored in a DataContainer. While FOOD_LEVEL will always exist on a player, this will not be the case for other values. The new getDefault and getDefaultValues allow a plugin to create such a value - crucially, with all additional data (e.g. minimum and maximum values) filled in. This ensures that a plugin can always add a proper ImmutableValue to a ChangeDataHolderEvent.ValueChange event, even if it doesn't yet exist on the target DataHolder.


TODO

This pull request is not yet complete. There are still many unresolved issues that need to be addressed:

  • ChangeDataHolderEvent.ValueChange, as the name suggests, only deals with values. Does it make sense to have an event for DataManipulator changes?
  • When writing the implementation of this PR, I discovered that's there's no simply way to convert a BaseValue into either a Value or ImmutableValue. While these are the only subclasses that currently exist, the API provides no guarantee that this will always be the case. Should we add convenience methods for getting a concrete Value/ImmutableValue from a BaseValue, or do we not want to commit to this in the API?
  • The current implementation of DataTransactionResult#get is inefficient, as it iterates over the entire list of values each time. Should we require that there can only be one ImmutableValue per key in a given DataCategory? Are the any circumstances where this would pose an issue?
  • Each DataCategory on DataTransactionResult sort of behaves like an immutable ValueContainer. Would it make sense to implement this interface on a 'view' into a DataTransactionResult?
  • DataTransactionResult#absorbResult has very little documentation, and (to me) gives unintuitive results. Is this method the right one to use when dealing with a ChangeDataHolderEvent.ValueChange? If not, what should be used instead? Under what circumstances would a plugin call this method?
  • Should DataHolder#offerWithEvent be:
  • Available alongside offer through the API?
  • Be the only option for plugins, and replace offer?
  • Be completely inaccessible to plugins, and only be used by the implementation?

Demonstration plugin

To demonstrate how these changes will work in practice, I've added support for firing ChangeDataHolderEvent.ValueChange for changes to food level (Keys.FOOD_LEVEL). This plugin shows how to use the event:

package org.spongepowered.test;

import static org.spongepowered.api.data.DataTransactionResult.DataCategory.REPLACED;
import static org.spongepowered.api.data.DataTransactionResult.DataCategory.SUCCESSFUL;

import org.spongepowered.api.data.DataTransactionResult;
import org.spongepowered.api.data.key.Keys;
import org.spongepowered.api.data.value.immutable.ImmutableValue;
import org.spongepowered.api.data.value.mutable.MutableBoundedValue;
import org.spongepowered.api.entity.living.player.Player;
import org.spongepowered.api.event.Listener;
import org.spongepowered.api.event.Order;
import org.spongepowered.api.event.data.ChangeDataHolderEvent;
import org.spongepowered.api.event.filter.cause.First;
import org.spongepowered.api.event.filter.data.GetKey;
import org.spongepowered.api.plugin.Plugin;
import org.spongepowered.api.text.Text;
import org.spongepowered.api.text.channel.MessageChannel;

import java.util.Optional;

@Plugin(id = "food-change-test", authors = "Aaron1011")
public class FoodChangeTest {

    @Listener(order = Order.EARLY)
    public void simpleListener(ChangeDataHolderEvent.ValueChange event) {
        Optional<ImmutableValue<?>> oldFood = event.getChanges().get(DataTransactionResult.DataCategory.REPLACED, Keys.FOOD_LEVEL);
        Optional<ImmutableValue<?>> newFood = event.getChanges().get(DataTransactionResult.DataCategory.SUCCESSFUL, Keys.FOOD_LEVEL);

        if (!oldFood.isPresent() || !newFood.isPresent()) {
            return;
        }

        MessageChannel.TO_ALL.send(Text.of(String.format("Simple listener: %s %s", oldFood.get().get(), newFood.get().get())));
    }

    @Listener
    public void getKeyListener(ChangeDataHolderEvent.ValueChange event,
            @GetKey(value = "FOOD_LEVEL", from = REPLACED) int oldFood,
            @GetKey(value = "FOOD_LEVEL", from = SUCCESSFUL) MutableBoundedValue<Integer> newFood,
            @First(tag = "a") Player player,
            @GetKey(value = "DISPLAY_NAME", tag = "a") Text name) {

        MessageChannel.TO_ALL.send(Text.of(String.format("GetKey listener: %s %s from player ", oldFood, newFood.get())).concat(name));

        event.setChanges(DataTransactionResult.builder().from(event.getChanges()).absorbResult(DataTransactionResult.successResult(newFood.asImmutable().with(newFood.getMaxValue()))).build());
    }

}

Aaron1011 added some commits Nov 12, 2017

Aaron1011 added some commits Nov 19, 2017

* @param <E> The type of value
* @return The created event, if available
*/
<E> Optional<ChangeDataHolderEvent.ValueChange> offerWithEvent(Key<? extends BaseValue<E>> key, E value, Cause cause);

This comment has been minimized.

@kashike

kashike Nov 19, 2017

Member

Why a Cause?

@kashike

kashike Nov 19, 2017

Member

Why a Cause?

@kashike kashike added the status: wip label Nov 19, 2017

/**
* Represents values that were successfully offered to the {@link DataHolder}
*/
SUCCESSFUL,

This comment has been minimized.

@liach

liach Nov 19, 2017

How about NEW?

@liach

liach Nov 19, 2017

How about NEW?

This comment has been minimized.

@Aaron1011

Aaron1011 Nov 19, 2017

Member

This corresponds to #getSuccessfulData, so it should be called SUCCESSFUL

@Aaron1011

Aaron1011 Nov 19, 2017

Member

This corresponds to #getSuccessfulData, so it should be called SUCCESSFUL

This comment has been minimized.

@liach

liach Nov 19, 2017

Does it also refer to the succeed data after replacement?

@liach

liach Nov 19, 2017

Does it also refer to the succeed data after replacement?

/**
* The id of the {@link Key} to use. This <b>must</b> be registered when the corresponding event listener
* for this annotation is registered.
* @return

This comment has been minimized.

@liach

liach Nov 19, 2017

Remove empty returns.

@liach

liach Nov 19, 2017

Remove empty returns.

* with {@link ChangeDataHolderEvent.ValueChange}
* @return
*/
DataTransactionResult.DataCategory [] from() default {DataTransactionResult.DataCategory.SUCCESSFUL};

This comment has been minimized.

@liach

liach Nov 19, 2017

Formatting

@liach

liach Nov 19, 2017

Formatting

@VapidLinus

This comment has been minimized.

Show comment
Hide comment
@VapidLinus

VapidLinus Nov 20, 2017

Please excuse me if this is a stupid question, but would the @GetKey event filter work for TargetEntityEvents as a shortcut to get key values from entities?

As an example:

// Prevent entities that aren't called Fred from being tamed
void onEntityTame(TameEntityEvent event, @GetKey("DISPLAY_NAME") Text name) {
    if (!name.toPlain().equals("Fred")) { event.setCancelled(true); }
}

I know you said "[the] event filter can also be used with a DataHolder", but I'm not sure what that implies.

VapidLinus commented Nov 20, 2017

Please excuse me if this is a stupid question, but would the @GetKey event filter work for TargetEntityEvents as a shortcut to get key values from entities?

As an example:

// Prevent entities that aren't called Fred from being tamed
void onEntityTame(TameEntityEvent event, @GetKey("DISPLAY_NAME") Text name) {
    if (!name.toPlain().equals("Fred")) { event.setCancelled(true); }
}

I know you said "[the] event filter can also be used with a DataHolder", but I'm not sure what that implies.

@parlough

This comment has been minimized.

Show comment
Hide comment
@parlough

parlough Jan 20, 2018

Member

Going to close this for now due to #1728 and SpongePowered/SpongeCommon#1670 which provide a method of implementing and listening to the event.

If you'd like to make other changes, open a new PR with those in relation to what is now in the API and implementation.

Member

parlough commented Jan 20, 2018

Going to close this for now due to #1728 and SpongePowered/SpongeCommon#1670 which provide a method of implementing and listening to the event.

If you'd like to make other changes, open a new PR with those in relation to what is now in the API and implementation.

@parlough parlough closed this Jan 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment