Properly implementing fluid tooltips

[idcppl]

What:
This PR allows for people to implement their own tooltips, with more than one line.

How solved:
This is solved by allowing for the user to pass a list through the argument, and storing them as a list.

Additional Info:
GTCE formula tooltips should still be put at index of one and show up in machine slots, and tank tooltips.

[warjort]

I don't think you should be breaking something in api like this?

Why not:

// Old method for backwards comaptibilty (in case somebody else is using it)
@Deprecated
String getFluidToolTip() {
// return first in list
}
// New method that is actually used (note the "s" on the end)
List<String> getFluidToolTips() {}

[warjort]

You are also not doing the same validation when registering a list:

 public static boolean registerTooltip(Fluid fluid, List<String> tooltip) {
        if (fluid != null && tooltip != null && !tooltip.isEmpty()) {

 public static boolean registerTooltip(Fluid fluid, String tooltip) {
        if (fluid != null && tooltip != null && !tooltip.isEmpty()) {

The old method checks for an empty tooltip while you are checking for an empty list of tooltips.

Why not just make the new method do:

 public static boolean registerTooltip(Fluid fluid, List<String> tooltips) {
        if (fluid != null && tooltips != null && !tooltips.isEmpty()) {
            tooltips.forEach(tooltip -> register(fluid, tooltip);
        }
  }

I also think the original validation for an empty tooltip is wrong.
It should instead be doing:

tooltip.trim().isEmpty()

[idcppl]

Thanks for the suggestions, fixed those area's. I didn't really think about how I was destroying backwards compatibility.

[warjort]

One more suggestion, I think the return value for the new method should be implemented something like:

 public static boolean registerTooltip(Fluid fluid, List<String> tooltips) {
        boolean result = false;
        if (fluid != null && tooltips != null && !tooltips.isEmpty()) {
            tooltips.forEach(tooltip -> {
                result = registerTooltip(fluid, tooltip) || result;
           });
        }
        return result;
    }

i.e. if any registration works it should return true.
I don't know how the result is actually used?

[idcppl]

I don't know why it's there either, but shouldn't I be avoiding unnecessary mutability? I'm just carrying on the return type from before. @DStrand1 can possibly let us know what he was thinking.

[warjort]

mutablility of local variables/parameters is fine

mutability is an issue when it is shared state across threads.

[serenibyss]

There was no significant motivation for providing return values for registration, other than that I did not want to throw an exception like other registries do when bad parameters are passed, so I wanted some form of a return code for the user of the API to have if needed. If I had a mistake in returning the wrong value, then it should be fixed. True should be returned on a successful registry, otherwise false.

Additionally, I am not sure why we are using Java's Iterable.forEach() when it has worse performance and worse readability in stacktraces.

[serenibyss]

Since we are doing a more flexible system for adding tooltips, it may be wise to have a registry freeze to prevent the tooltip registry from being modified further past a certain point in the Forge loading cycle. If you need help implementing this, I can do it

[warjort]

Additionally, I am not sure why we are using Java's Iterable.forEach() when it has worse performance and worse readability in stacktraces.

It's swings and roundabouts.

Iterable.forEach() can be optimised by the implementation in ways for(x : Iterable) cannot.
The forEach() does introduce an extra function call to do the lamba versus for() when there is no optimisation and lambas
have additional restrictions on what they can do.

In the above example using a for() would probably be the better choice since we know the implementation
is defined in the same class as an ArrayList (which has no optimisations - at least for Oracle/OpenJDK).

[idcppl]

Should we freeze the registry, and if so, should it be a GTControlledRegistry? And what would the index cap be on the registry if it were to be GTControlledRegistry? Mostly a question for Lag.

Quick edit: There also RegistrySimple we could use, doesn't have an max index requirement.

[warjort]

What issue does freezing the registry solve?

Is there something like block or color registration where you will "miss the bus" if you register too late?

Or the other use of registry freezing is to write the registry keys into the save file. So that forge can warn if they are missing in a future load of the save.

[LAGIdiot]

I don't feel like there is any reason for us to freeze this as there would not be "anything missing" if some one adds tooltip later.

[serenibyss]

Please use TextFormatting as ChatFormatting only exists on the Client, and since we are now setting this from a shared class, this will cause a NoClassDefFoundError on dedicated servers

[serenibyss]

Shouldn't this be .get(0)? From how we are registering it in registerTooltip(Fluid, String), it does not appear to be skipping the 0 index

[LAGIdiot]

I am too bit concerned why we are using sometimes 0, sometimes 1 when working with tooltips.

[serenibyss]

I believe it is because when applying the tooltip to an ItemStack, the name of the Item is in index 0 in the list. For fluid tooltips, since I had to do it a bit more roundabout, index 0 of the registry will be put at index 1 of an ItemStack containing this fluid. Otherwise, when given to a FluidStack in JEI, it will just simply add to the end of the tooltip list rather than insert at an index.

[serenibyss]

I think we should change this to be 2 different methods of handling it, since our fluid tooltip is not just a chemical formula anymore, but a general fluid tooltip application