Refactor slider widgets to share a common base class, add int and long slider lists

[md5]

The corresponding entry objects will be configured by AutoConfig when @ConfigEntry.Gui.BoundedDiscrete is present for fields of the following types:

• IntegerSliderListEntry: List<Integer>, Integer[], int[]
• LongSliderListEntry: List<Long>, Long[], long[]

~I would note that I pretty much just copied and pasted IntegerSliderListEntry to create LongSliderListEntry and once I was done all the differences between the two entries, their cells, and builders were minimal, so it feels like they could have a more DRY implementation.~ Also, it's seems likely that more of the implementation could be shared between the normal slider entries and the slider list entries, but I didn't try to achieve that ~either~.

One final question I have is regarding copyright notices on files, which I did not add. If you want to let me know what you want to do there, I can amend this PR. I'm assuming you don't have a formal CLA, but I'm happy contributing these changes under the stated LGPL license.

Here's an example of what this slider list looks like in the config screen of my mod:

[md5]

I ended up spending some more time on this code and refactored IntegerSliderEntry and LongSliderEntry to have a common base class called AbstractSliderEntry, similar to the refactoring that I did in the evolution of this PR. I also combined the two Slider private classes into a single shared classed called ManagedSliderWidget. I didn't include those commits in this branch but they are here:

• Combine IntegerSliderEntry and LongSliderEntry: https://github.com/appropriate/cloth-config/commit/cdb940ba36f3c10ca426e0f4ea35b2f736a88454
• Combine Slider classes: https://github.com/appropriate/cloth-config/commit/5fca7d27704a70b169b26a98d6c1d78dc31f51bf

I also started playing with an EnumSliderEntry based on AbstractSliderEntry, but that isn't ready to push yet.

[md5]

Here's another commit of interest that updates TooltipListEntry to take Function<T, Optional<Component[]>> tooltipGetter in preference to Supplier<Optional<Component[]>>: 17ce11f2a515d14aff1cfd444861af7310ecdb75

This allowed me to remove all of the entry.setTooltipSupplier(() -> tooltipSupplier.apply(entry.getValue())) calls in the builders, which in turn made the implementation of EnumSliderEntry (which I'm still testing) more straightforward. Making this change moves more toward passing all arguments through the Entry constructors, which I believe fits better with the builder pattern.

Update: Got EnumSliderEntry working reasonably too in https://github.com/appropriate/cloth-config/commit/dcddf74753c59590a2c49b6d63f5211a1ffe2ed9 (full diff including this PR and all other commits mentioned previously: https://github.com/shedaniel/cloth-config/compare/v4...appropriate:v4-enum-slider-entry)

https://user-images.githubusercontent.com/76367/110228842-44028f00-7eb9-11eb-96ea-97d98e9a1a98.mov

[shedaniel]

I don't have a CLA, setting one up is beyond my reach. However I do require you to give permission to the content of this PR to me, into LGPL. Please apply the license header for that.

This PR should be good to merge after that.

[md5]

I don't have a CLA, setting one up is beyond my reach. However I do require you to give permission to the content of this PR to me, into LGPL. Please apply the license header for that.

This PR should be good to merge after that.

Cool, I can add the license headers to any new files.

Do you want me to include the other changes from https://github.com/appropriate/cloth-config/commit/cdb940ba36f3c10ca426e0f4ea35b2f736a88454, https://github.com/appropriate/cloth-config/commit/5fca7d27704a70b169b26a98d6c1d78dc31f51bf, or https://github.com/appropriate/cloth-config/commit/17ce11f2a515d14aff1cfd444861af7310ecdb75?

In particular, I think the tooltipGetter refactoring in 17ce11f2a515d14aff1cfd444861af7310ecdb75 improved things quite a bit. I could see a similar refactor with createNewCell being helpful since there are cases right now where a superclass is calling that function before the entry subclass is fully initialized (and hence can have empty final fields, complicating new cell construction that relies on constructor parameters of the entry subclass).

[shedaniel]

Yes for the first two commits, the third commit (appropriate@17ce11f) would break binary compatibility as we don't have a clear api and impl split. (which I will work on)

[md5]

Yes for the first two commits, the third commit (appropriate/cloth-config@17ce11f) would break binary compatibility as we don't have a clear api and impl split. (which I will work on)

Are you specifically talking about the removal of the protected tooltipSupplier field itself? I did add constructors to allow any subclasses that don't access that field directly to continue working, but you're right about any subclass that directly accesses tooltipSupplier. It seems very unlikely to me that there are subclasses doing that, but I understand the desire to maintain strict binary compatiblity.

It would probably be possible to maintain a shadow tooltipSupplier field that is kept in sync with tooltipGetter to avoid that breakage. Do you feel like that's worthwhile or not? All I can say in favor of including the change is that I was having a really hard time implementing EnumSliderList without it, but I don't recall exactly why at the moment.

[md5]

Actually, it looks like tooltipSupplier was private, so I'm not sure what binary incompatibility you're referring to. I also thought I hadn't removed or changed the access modifiers of any methods, though some methods may have moved up the class hierarchy.

Have you run a report that shows specific binary incompatibilities introduced by these changes? I tried playing around with japicmp a bit, but had trouble getting what I thought was reasonable output from it.

[md5]

@shedaniel now that 1.17 is out and I see that you've released a version of clothconfig2 from the v5 branch, I'm wondering if it's worth revisiting the work I did in this PR. I haven't looked into the build failures I'm getting closely yet, but it looks like there were some upstream breaking changes in Minecraft that need to be addressed to get this working again.

[md5]

Closing this to get it off my list of open PRs. Please let me know if you're interested in revisiting this work and I'll see what I can do to help