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

Update MixinGuiChat_OpenLinks.java #441

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Update MixinGuiChat_OpenLinks.java #441

merged 2 commits into from
Nov 20, 2024

Conversation

mitchej123
Copy link
Contributor

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

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
@mitchej123 mitchej123 requested review from Alexdoru and a team November 18, 2024 23:34
@reallydrained
Copy link

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
Copy link
Member

Another reason go not use AT 🤡

Copy link
Member

@Alexdoru Alexdoru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mitchej123
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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.

@mitchej123 mitchej123 requested a review from a team November 20, 2024 07:41
@Glease
Copy link
Contributor

Glease commented Nov 20, 2024

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
Copy link
Member

I did it

@Dream-Master Dream-Master merged commit b2fa0bd into master Nov 20, 2024
1 check passed
@Dream-Master Dream-Master deleted the patch-visbility branch November 20, 2024 11:39
@reallydrained
Copy link

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

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

Successfully merging this pull request may close these issues.

5 participants