Use Modern Adventure + Minimessage instead of Legacy color-codes

MarkusTieger commented 2 months ago

Motivation

Solves https://github.com/CloudNetService/CloudNet-v3/issues/1384

Modification

Uses the "Component" internally instead of the legacy color codes
Asks in the initial setup if you want to use minimessage instead of legacy color codes.
Parses Messages with Minimessage or LegacyComponentSerializer depending on your choice.
Generates defaults with Minimessage or LegacyComponentSerializer depending on your choice.
Changed the placeholder in messages to "" instead of "%name%"

Result

Now the codebase is more clean and you can use the modern adventure api and minimessage format instead of the old legacy one.

Other context

Only partially tested

0utplay commented 2 months ago

I really dont like the approach of this pull request. I feel like we should not break existing configs and furthermore should not enforce minimessage. For such huge changes you should ask beforehand

MarkusTieger commented 2 months ago

I really dont like the approach of this pull request. I feel like we should not break existing configs and furthermore should not enforce minimessage. For such huge changes you should ask beforehand

It doesn't enforce minimessage as it is optional (you will be asked on initial setup) and it won't break existing configs (with some exceptions, but I could make a commit to fix these) because the default value is set to false (meaning legacy color codes), if the cloud is upgraded.

MarkusTieger commented 2 weeks ago

Will there be a reply? @0utplay

0utplay commented 2 weeks ago

Sorry, but what do you want to hear? I am still not conviced that this is the right approach for supporting minimessage. I feel like this can be solved much less invasive. Furthermore you said its only partially tested, the codestyle does not align with our codestyle set in the intellij project config and checkstyle. Furthermore there even are some unresolved merge conflicts in the code

MarkusTieger commented 2 weeks ago

Sorry, but what do you want to hear? I am still not conviced that this is the right approach for supporting minimessage. I feel like this can be solved much less invasive. Furthermore you said its only partially tested, the codestyle does not align with our codestyle set in the intellij project config and checkstyle. Furthermore there even are some unresolved merge conflicts in the code

What would be the right approach for supporting minimessage in your opinion? Would it change something if I would test it extensively? It seems like you are not liking how I implemented this change anyway? I fixed the merge conflicts already a few minutes ago. (And fixed the compile error. I didn't notice that limbo support was added) The checkstyle does pass in gradle.

0utplay commented 2 weeks ago

Sorry, but what do you want to hear? I am still not conviced that this is the right approach for supporting minimessage. I feel like this can be solved much less invasive. Furthermore you said its only partially tested, the codestyle does not align with our codestyle set in the intellij project config and checkstyle. Furthermore there even are some unresolved merge conflicts in the code

What would be the right approach for supporting minimessage in your opinion? Would it change something if I would test it extensively? It seems like you are not liking how I implemented this change anyway? I fixed the merge conflicts already a few minutes ago. The checkstyle does pass in gradle.

Doing all of that won't solve the problem I have with this approach. You are right there. I think that we should leave all configs and so on using strings and when sending messages to the console / players / motd and so on, just convert them on the fly. Currently our ComponentFormats like the one for adventure: AdventureComponentFormat have a method that just takes the string and constructs a component from it. Why can't we implement minimessage there?

I know that some parts of cloudnet are still not using our ComponentFormats, but I would rather extend the formats to these places than rewrite half of the cloud

MarkusTieger commented 2 weeks ago

Sorry, but what do you want to hear? I am still not conviced that this is the right approach for supporting minimessage. I feel like this can be solved much less invasive. Furthermore you said its only partially tested, the codestyle does not align with our codestyle set in the intellij project config and checkstyle. Furthermore there even are some unresolved merge conflicts in the code

What would be the right approach for supporting minimessage in your opinion? Would it change something if I would test it extensively? It seems like you are not liking how I implemented this change anyway? I fixed the merge conflicts already a few minutes ago. The checkstyle does pass in gradle.

Doing all of that won't solve the problem I have with this approach. You are right there. I think that we should leave all configs and so on using strings and when sending messages to the console / players / motd and so on, just convert them on the fly. Currently our ComponentFormats like the one for adventure: AdventureComponentFormat have a method that just takes the string and constructs a component from it. Why can't we implement minimessage there?

I know that some parts of cloudnet are still not using our ComponentFormats, but I would rather extend the formats to these places than rewrite half of the cloud

With that implementation there would still be a problem. Bedrock doesn't support Hex and all java players under 1.16 don't do either. My implementation does checks on each platform for that. That means we either just passthrough hex, which could be rendered incorrectly on some platforms or we add a parameter to each method which says if hex should be used. But in that case, you are already rewritting half of the cloud. Technically this is a problem which also exists in the current CloudNet-v4 version, but then you wouldn't be able to use tags like "\<rainbow>", "\<gradient>" etc. and this is an issue which should be addressed anyway. Storing it as strings also means you can't use custom fonts from resource packs, as the "\<font>" will not be serialized. (Not that I think "\<font>" will be used anyway. But I think the "\<hover>" or "\<click>" tag might be used).

I think even with the usage of strings, which I think is a more messy way, it would probably be round about the same amount of changes to the existing codebase. As CloudNet-v4 isn't released yet, such a breaking change in the api shouldn't be a problem?

CloudNetService / CloudNet