Properly implementing fluid tooltips #1581

idcppl · 2021-04-23T02:50:46Z

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 · 2021-04-23T08:07:53Z

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 · 2021-04-23T08:13:25Z

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()

…uggested changes.

idcppl · 2021-04-23T08:26:18Z

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

… still in use.

warjort · 2021-04-23T08:56:40Z

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 · 2021-04-23T09:02:41Z

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 · 2021-04-23T09:05:54Z

mutablility of local variables/parameters is fine

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

serenibyss · 2021-04-23T09:08:12Z

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 · 2021-04-23T09:12:06Z

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 · 2021-04-23T09:25:37Z

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 · 2021-04-23T09:26:20Z

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 · 2021-04-23T09:32:32Z

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 · 2021-05-04T17:46:32Z

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 · 2021-05-08T08:59:47Z

src/main/java/gregtech/common/MetaFluids.java

@@ -145,12 +146,12 @@ public static void init() {
                int temperature = fluidMaterial.getFluidTemperature();
                Fluid fluid = registerFluid(fluidMaterial, FluidType.NORMAL, temperature);
                fluidMaterial.setMaterialFluid(fluid);
-                FluidTooltipUtil.registerTooltip(fluid, fluidMaterial.chemicalFormula);
+                FluidTooltipUtil.registerTooltip(fluid, ChatFormatting.GRAY + fluidMaterial.chemicalFormula);


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 · 2021-05-08T09:01:55Z

src/main/java/gregtech/api/util/FluidTooltipUtil.java

+        if (fluid == null)
+            return null;
+
+        return tooltips.get(fluid).get(1);


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

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

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 · 2021-05-08T09:03:10Z

src/main/java/gregtech/common/ClientProxy.java

@@ -154,33 +152,33 @@ public static void addMaterialFormulaHandler(ItemTooltipEvent event) {

        // Handles Item tooltips
        if (!(itemStack.getItem() instanceof ItemBlock)) {
-            String chemicalFormula = null;
+            List<String> chemicalFormula = null;


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

idcppl added 2 commits April 22, 2021 20:02

Properly implementing FluidTooltipUtil

1beea21

Moved tooltips to the top of the list

7e15068

Fixed API that would be broken by this change and added some of the s…

58558d6

…uggested changes.

idcppl added 2 commits April 23, 2021 03:33

Used 1 instead of 0 for index.

ae5b173

Moved more tooltip indexes to 0 and still had some deprecated methods…

ee49b94

… still in use.

Display names are at 0, and fixed foreach loop.

2be90ff

Missed an index 0, :(

7b7f43e

LAGIdiot added rsr: revision Release size requirements: Revision type: feature New feature or request labels May 4, 2021

serenibyss requested changes May 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly implementing fluid tooltips #1581

Properly implementing fluid tooltips #1581

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

serenibyss commented Apr 23, 2021

serenibyss commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021 •

edited

Loading

warjort commented Apr 23, 2021

LAGIdiot commented May 4, 2021

serenibyss May 8, 2021 •

edited

Loading

serenibyss May 8, 2021

LAGIdiot May 23, 2021

serenibyss May 23, 2021

serenibyss May 8, 2021

Properly implementing fluid tooltips #1581

Are you sure you want to change the base?

Properly implementing fluid tooltips #1581

Conversation

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021

warjort commented Apr 23, 2021

serenibyss commented Apr 23, 2021

serenibyss commented Apr 23, 2021

warjort commented Apr 23, 2021

idcppl commented Apr 23, 2021 • edited Loading

warjort commented Apr 23, 2021

LAGIdiot commented May 4, 2021

serenibyss May 8, 2021 • edited Loading

Choose a reason for hiding this comment

serenibyss May 8, 2021

Choose a reason for hiding this comment

LAGIdiot May 23, 2021

Choose a reason for hiding this comment

serenibyss May 23, 2021

Choose a reason for hiding this comment

serenibyss May 8, 2021

Choose a reason for hiding this comment

idcppl commented Apr 23, 2021 •

edited

Loading

serenibyss May 8, 2021 •

edited

Loading