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

Problem with component class #37

Closed
Nightenom opened this issue Nov 19, 2024 · 5 comments
Closed

Problem with component class #37

Nightenom opened this issue Nov 19, 2024 · 5 comments

Comments

@Nightenom
Copy link

Hello,
could you please consider either extending MutableComponent or even better using and extending ComponentContents instead (that's what vanilla do)? Thank you
Related crash: ldtteam/BlockUI#104

@Swedz
Copy link
Owner

Swedz commented Nov 19, 2024

The reason I use Component instead of MutableComponent is because my TextLines are not meant to be mutable. Making them MutableComponents and retaining my current functionality I have set for them would certainly create more issues where people expect them to be mutable when they technically aren't. It would take quite a bit of changes on my end, but its not that unreasonable. Using ComponentContents doesn't allow me to use styling and to add my TextLines directly to tooltips without having to use extra bloat to convert them to Components.

I'm not entirely opposed to making this change of making TextLines mutable, but it might be better practice if BlockUI didn't assume all Components are MutableComponents here. Something like text instanceof MutableComponent mutable ? mutable : text.copy() would be simple enough I think?

@Nightenom
Copy link
Author

Nightenom commented Nov 19, 2024

I mean technically blockui can be only on Component, but consumer mods would have to cast every getter + it's vanilla design to have everything as MutableComponent and ComponentContents, which if implemented using record are immutable (note we are using it ourselves like this, but only for our UI components - eg. https://github.com/ldtteam/BlockUI/blob/version/main/src/main/java/com/ldtteam/blockui/util/SpacerTextComponent.java)
Could you please describe what is the purpose of your TextLine classes? Looking at code I don't see anything that would make TextLine any different compared to Component.translatable (apart from immutability)

@Swedz
Copy link
Owner

Swedz commented Nov 19, 2024

I was saying the highlighted input methods cast from Component to MutableComponent without any check of any kind in AbstractTextElement. I wasn't recommending you change from using MutableComponents to Components lol.

The purpose of the TextLine classes are to simplify component formatting. It uses custom parsers and some examples of those can be seen here and here. TextLine itself uses Component so it can be used directly without having to do additional conversions like you do in your SpacerTextComponent. Here's an example of what I mean by that.

@Swedz
Copy link
Owner

Swedz commented Dec 10, 2024

I don't plan on making this change so I'm closing this. If you want to fix this, you can do the one line change solution I presented in a previous comment. :)

@Swedz Swedz closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2024
@Nightenom
Copy link
Author

Firstly, thank you for insights on your code, well, surely we can add the casting
Secondly and nevertheless I still don't see the point behind your design (I would eg. understand if it would be for delayed or dynamic arg evaluation) imho same could be done using plain builder approach which would even eliminate the need for extending the Component at all. In any other case you could use ComponentContents because it was also designed for things like lazy eval etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants