Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop 'Text' prefix in text model #174

Closed
jamierocks opened this issue Oct 28, 2018 · 13 comments · Fixed by #650
Closed

Drop 'Text' prefix in text model #174

jamierocks opened this issue Oct 28, 2018 · 13 comments · Fixed by #650

Comments

@jamierocks
Copy link
Contributor

Currently the net/minecraft/text has all of the text model components prefixed with TextComponent, in contrast with Mojang's own naming scheme (XComponent).

Its important to note that only TextComponent directly represents actual text, the rest are just placeholders apart a model that is constructed into text client-side. IMO the Text prefix is verbose and unnecessary.

I propose that we should use Mojang's naming (where known) and the following changes:

  • net/minecraft/text/ITextComponent -> net/minecraft/text/Component (this is kind of given away in some of the serialisation code).
@asiekierka
Copy link
Contributor

I propose that we should use Mojang's naming (where known)

That has always been a bit of a controversial point: some Fabric developers argue the names should be improved upon where applicable instead. I, personally, believe that while the official names are an useful hint, we should treat them as just that - a hint. Additionally, I don't believe the names in toString always reflect the actual class name.

Anyhow, I'm opposed, but I'm a fan of verbosity.

  • "Component" is a common keyword which can easily be imagined to occur in a lot of modded content patterns, potentially even not far from chat message-emitting code.
  • Using or not using "I" to mark interfaces is, again, a point of debate - though I thought we generally settled on using it, with perhaps the exception of direct derivatives from Java interfaces (say, IntIterable extends Iterable).

@sfPlayer1
Copy link
Collaborator

Fabric tends to use common prefixes all over the place, which is quite useful for IDE's auto completion and recognition. Not using Text* wouldn't fit everything else.

I'd rather drop the "Component" infix, it is technically wrong (the "component" usually references the whole thing, not a part ("component") of something) and doesn't add to readability.

@jamierocks
Copy link
Contributor Author

"Component" is a common keyword which can easily be imagined to occur in a lot of modded content patterns, potentially even not far from chat message-emitting code.

That's a fair point, though I don't think it'd make too big of a difference (its rarely imported) - the subclasses (Text, Score, etc) I really doubt could be apart another system so I don't think that's so much of an issue.

Using or not using "I" to mark interfaces is, again, a point of debate - though I thought we generally settled on using it, with perhaps the exception of direct derivatives from Java interfaces (say, IntIterable extends Iterable).

Well that's a different discussion, I have my preference but that can be discussed elsewhere - for the purposes of this discussion I'd be happy with either Component or IComponent.


Fabric tends to use common prefixes all over the place, which is quite useful for IDE's auto completion and recognition. Not using Text* wouldn't fit everything else.

Really? You'd certainly still get completion for TextComponent, and I would imagine the others too (given the package, but I guess that depends on IDE).

I'd rather drop the "Component" infix, it is technically wrong (the "component" usually references the whole thing, not a part ("component") of something) and doesn't add to readability.

Just because in a lot of cases only a single component is used, doesn't discount that many can be used (and frequently are - e.g. Press <x> button to jump). Component is an essential part of the name.

This thing is, while this is certainly apart the text model (hence net/minecraft/text) only one component (TextComponent) is actual text - the rest are placeholders for the client to fill-in. IMO it doesn't make too much sense to repeat the package for the components in the class name.

@sfPlayer1
Copy link
Collaborator

They are all representing localizable text, usually to be displayed on the client. "Text" conveys naturally that it can be embedded into itself.

If you think about it from the other direction, the "component" has no real container, it is always constrained to itself until it gets consumed or exported into a string. "Component" is an unnecessary distinction towards something that doesn't exist. Stripping "Text" makes it as generic and meaningless as "Element", "Part" or similar, losing the description of its identity/purpose.

@asiekierka
Copy link
Contributor

About the package argument: I'm of the belief that you don't really look at package names when reading code...

@jamierocks
Copy link
Contributor Author

About the package argument: I'm of the belief that you don't really look at package names when reading code...

Sure, when reading code context is where you'd know its text related - package is only really relevant when importing. Though importing is the only case I see it being an issue anyhow...

// Even when using Component in a field, context easily allows you to
// know its text-related
final Component message = new TextComponent("Hello, World!");
player.message(message);

// Each of the components are fairly easily identifiable as text
player.message(new KeybindComponent("key.jump"));
player.message(new TranslatableComponent("info.kick", "player-x"));
player.message(new ScoreComponent("name", "objective"));
player.message(new SelectorComponent("pattern"));

Take a look at the examples for kyori/text, to see that even without the 'Text' prefix its still very easy to see what it means and looks a bunch cleaner too (IMO ofc).

@asiekierka
Copy link
Contributor

You might have a point with the code examples, though I'd still argue ScoreComponent/SelectorComponent/KeybindComponent can be confusing in certain contexts.

And I still wouldn't be so sure on calling the base "Component". Rubs me the wrong way.

@jamierocks
Copy link
Contributor Author

And I still wouldn't be so sure on calling the base "Component". Rubs me the wrong way.

Maybe, but it's not uncommon - take org.w3.dom.Node for example.

@strikerrocker
Copy link

I wouldn't removing Text prefix as Component alone wilk cause confusion as there are many similar class with same names exists

@falkreon
Copy link

Okay but let's look at what happens in the reverse:

// Even without Component in the name, names are still focused on the object's use
// Note that I've reversed from StringText to TextString because it flows more naturally
// in english
final Text message = new TextString("Hello, World!");
player.message(message);

// Each of these objects' purposes are easily determined without the word "Component"
player.message(new KeybindText("key.jump"));
player.message(new TranslatableText("info.kick", "player-x"));
player.message(new ScoreText("name", "objective"));
player.message(new SelectorText("pattern"));

Seems legit to me.

@jamierocks
Copy link
Contributor Author

Well, as kashike reminded me - Mojang do not call them text components - they call them chat components (you just need to look at the changelogs to know this).

They also don't refer to the text component as a string text component - text for them, is what fabric calls string.

@Prospector
Copy link
Contributor

Yet another confirmation mojang calls them components
image

@jamierocks
Copy link
Contributor Author

jamierocks commented Mar 28, 2019

My research led me that are in a chat package in network for what's it's worth -> https://github.com/jamiemansfield/mcmap/tree/18w50a/workspace/net/minecraft/network/chat

(Edit: Silly mobile)

@jamierocks jamierocks reopened this Mar 28, 2019
jamierocks added a commit to jamierocks/pomf that referenced this issue Apr 24, 2019
Closes FabricMC#174, Succeeds FabricMC#567 (partially).

This PR brings the naming of chat components in line with Mojang - in both
package and *chat* components.
modmuss50 pushed a commit that referenced this issue May 5, 2019
Closes #174, Succeeds #567 (partially).

This PR brings the naming of chat components in line with Mojang - in both
package and *chat* components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants