Scoreboard objective number format api

[davidmayr]

This PR introduces an easily accessible number format API (using adventure styles and components) for scoreboard objectives for a feature introduced in 1.20.3.

Setting a number format can be as easy as this:

//Remove numbers
objective.numberFormat(NumberFormat.blank());

// Style numbers in green and bold
objective.numberFormat(NumberFormat.styled(NamedTextColor.GREEN, TextDecoration.BOLD));

//Replace numbers
objective.numberFormat(NumberFormat.fixed(Component.text("Lol")));

As for the implementation: The conversion between the Adventure style and Mojangs implementation is a bit messy. The only other way I can think of to implement this is manually converting every style property. As paper doesn't even do that with chat components either, I have decided to just serialize and deserialize again.

[yannicklamprecht]

Like that. Yes, we're sealed know. 🥳

[456dev]

In vanilla, you can set it per each score holder, right? How is that done here?

Unless I'm misunderstanding what the "objective" is

[davidmayr]

In vanilla, you can set it per each score holder, right? How is that done here?

Unless I'm misunderstanding what the "objective" is

I totally missed that that's possible in vanilla. You can set it for the entire objective (e.g. your entire sidebar) or apparently per entry as well (which I missed)

I will add that asap

[davidmayr]

That's implemented now. With the latest commit, you can change the formatting of individual scores or the whole objective

[jpenilla]

Should use our codecs for conversions, not the GsonComponentSerializer, see the minecraft round trip style test in AdventureCodecsTest for an example. Also put the conversion methods at PaperAdventure.asVanilla/asMinecraft to be consistent.

[Machine-Maker]

All there records should be package-private and not exposed to the API. Instead, have interfaces with a method for each record parameter. The field for the blank format can be on the NumberFormat interface.

[davidmayr]

Should use our codecs for conversions, not the GsonComponentSerializer, see the minecraft round trip style test in AdventureCodecsTest for an example. Also put the conversion methods at PaperAdventure.asVanilla/asMinecraft to be consistent.

So just we are on the same path: I should add a asVanilla method for the style conversion to PaperAdventure? Or do you mean the entire number formatting? (Probably not, but just making sure)

Aaaand should I update the adventure patch? Because as far as I can tell, everything inside that class comes from that patch and if I would put it in this one it could break multiple patches if this one would be removed (e.g. temporarily while upgrading), right?

[davidmayr]

All there records should be package-private and not exposed to the API. Instead, have interfaces with a method for each record parameter. The field for the blank format can be on the NumberFormat interface.

Sorry, but I don't think I really get what you are asking me to do. 😅

As far as I can tell, you want me to either:
Package private the entire number format implementations and add static methods to the base interface for creation. But that would be an issue for accessing the number format value, wouldn't it?

You could theoretically also have meant to only package private the constructors, but I don't think that's the case because private constructors don't exist in records, if I recall correctly? - sorry if I'm wrong, I'm using Kotlin most of the time 😅

Or you want me to:
Create interfaces for every type and make the implementations private (but still in the API because I don't know of any provider service to construct it?) But I don't really get the point of that? Wouldn't that just be useless class clutter?

[Machine-Maker]

Aaaand should I update the adventure patch? Because as far as I can tell, everything inside that class comes from that patch and if I would put it in this one it could break multiple patches if this one would be removed (e.g. temporarily while upgrading), right?

Yeah, you can make asVanilla/asAdventure methods inside PaperAdventure that use the codec's and they should be in the adventure patch.

Or you want me to:
Create interfaces for every type and make the implementations private (but still in the API because I don't know of any provider service to construct it?) But I don't really get the point of that? Wouldn't that just be useless class clutter?

Yes, exactly this. interfaces for NumberFormat, StyledFormat, FixedFormat (no need for blank format, should just use an equality check to know that). And then package-private records that implement those interfaces with static methods on NumberFormat to create instances of each one.

There are a couple reasons for this. 1) javadocs. can have better, separated javadocs for the fluent getters and factory methods and types if you separate out the record from the API interface. 2) future changes. Changing records and making them maintain compat is super ugly if something changes in the future. If the constructor is public, you have to always keep the old one and make new ones and you have to have old components... its just not fun. Look at StructuresLocateEvent$Result and observe the reason why its not good to do that. Better to have static methods to deprecate and we can just change whatever we want in the record.

[yannicklamprecht]

After some thoughts I probably would be against the nullability in the setter and just add a Reset record that is also sealed.
What do you think of @Machine-Maker, @jpenilla, @MenschenToaster ?

[Machine-Maker]

If we didn't want to have nullability in the setter, I would prefer a clear method instead. I don't think adding a type for resetting is a good idea.

[davidmayr]

no need for blank format, should just use an equality check to know that

Internally it makes sense, but for accessibility when querying the current format it can get confusing and just more difficult to check, right?

If somebody wants to know if it's a blank format, they would have to do:
if(!(format instanceof StyledFormat) && !(format instanceof FixedFormat)) {}
to know if it's a blank format or not. And if there would be a new format in the future, their code would be broken

[Machine-Maker]

By "equality check" I meant you can do

if (NumberFormat.blank().equals(format)) {}

[davidmayr]

By "equality check" I meant you can do

if (NumberFormat.blank().equals(format)) {}

That makes way more sense. Sure, then I'll do that

[lynxplay]

Requires testing and javadoc addition regarding the creation of the score on the numberFormat setter, otherwise LGTM.

Given upstream has yet to release even a PR for this, I think it is fine to move forwards with this sooner than later 👍

[davidmayr]

Requires testing

Did some basic testing. I only tested the number format on the score and not on objectives. But the original code in my first commit was tested on the objective in another plugin as well. (Because that code is used in a paper fork of mine as of right now) I would assume that works.

Getting (and therefor converting the format back) works as well (which can be sen by the output in the console)
Some further testing might be required but as far as I can see everything works as intended

[davidmayr]

Any updates?

[Cryptite]

Might be nice to update the OP which provides examples of the API.

For example, fixed number format is now objective.numberFormat(NumberFormat.fixed(Component.text("Lol"));

[lynxplay]

Done 👍

[davidmayr]

Done 👍

We both edited the description at the same time 🙃
Didn't even know maintainers could edit the PR's description 😅