NucleoidMC / Server-Translations

MIT License
42 stars 20 forks source link

Apply SystemDelegatedLanguage as reload listener for compatibility with mods wrapping the language instance #43

Closed Fuzss closed 1 year ago

Fuzss commented 1 year ago

Hi there, I saw you are already aware of the issue my Universal Enchants mod and STAPI were having. I've now 'fixed' that issue by simply disabling the conflicting feature in Universal Enchants when STAPI is present, but that's not really a solution honestly and I'd very much like to revert my change and rather see a solution on STAPI's end.

So I was wondering why there is a need to use this mixin?

I think simply using a reload listener on the client side (for client resources) would be much more elegant, as it's guaranteed to only run once per reload and achieves the same behavior you are going for right now with your mixin. There's also no danger of some infinite loop like the one with my mod happening again. This is also the approach I use in my mod. You implementing your language wrapper the same way would make both mods compatible out of the box which would be great.

So I've gone ahead and converted your implementation to use a resource reload listener.

Right now though there is one problem with that when another mod wrapping the language instance (like mine) is present: MutableTextMixin and TranslatableTextContentMixin break the whole text engine as they expect your language wrapper to be what is set as the current language (which cannot be guaranteed when other mods are wrapping the language instance, too). Honestly though I do not get the point of these two mixins, from my understanding both simply recreate the exact same behavior of the respective methods they are trying to override/enhance (just in a very weird way, but the methods still seem to do the exact same thing as the underlying vanilla logic). Removing both mixins does not result in any difference at all for me. What were you trying to achieve with those two mixins? If those mixins are indeed necessary that wouldn't be a problem to work around though, but as mentioned I don't see them doing anything at all right now, so that's why I removed them.

thecatcore commented 1 year ago

@Patbox can you include this refactor too in 2.0?

Patbox commented 1 year ago

@Patbox can you include this refactor too in 2.0?

It doesn't apply anymore. Refactor changed up things and removed static delegated language, which should solve this issue anyway

thecatcore commented 1 year ago

Awesome, then closing this