GTNewHorizons / Hodgepodge

A HodgePodge of patches
GNU Lesser General Public License v3.0
39 stars 62 forks source link

Update MixinGuiChat_OpenLinks.java #441

Closed mitchej123 closed 1 week ago

mitchej123 commented 1 week ago

Saw this error floating around in the wild: Caused by: org.spongepowered.asm.mixin.transformer.throwables.InvalidMixinException: PRIVATE @Overwrite method func_146407_a in mixins.hodgepodge.early.json:minecraft.MixinGuiChat_OpenLinks from mod hodgepodge cannot reduce visibiliy of PROTECTED target method

Apparently someone's AT'd the field and made it protected.... haven't tested this (commiting from Github) so please verify before merging

reallydrained commented 1 week ago

I tested this with the artifact .jar and it fixed the issue =D (I was the one who reported the error/crash log)

Could only use v2.5.71 or lower before without it crashing but now no crash!

Alexdoru commented 1 week ago

Another reason go not use AT 🤡

mitchej123 commented 1 week ago

To solve that issue you should instead of the overwrite inject at head and always cancel

go look at the git blame of who did the original fix :P

Alexdoru commented 1 week ago

I know it's me that did the original fix. It's only now broken because of an external factor which is someone using AT. This example is one more reason mixin accessors and invokers are superior.

Also blaming me for the original code doesn't make my review less valid. In your current fix you make it protect but now it will crash too if another AT makes it public. So your fix doesn't actually fix the underlying problem.

mitchej123 commented 1 week ago

It solves the crash. I'm on vacation and I'm not going to do a more involved fix. I'm also fine just making it public, I don't need to do too much work to work around this crash.

Glease commented 1 week ago

So @Alexdoru can you maybe open a new PR to implement what you mentioned? since mitch is obviously too occupied to follow up on this.

If neither of you have time, I might be able to do something later

Alexdoru commented 1 week ago

I did it

reallydrained commented 1 week ago

The new version (Build and test #1158) also works, no crash!