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

Fix ModMenu compatibility #5

Closed
wants to merge 1 commit into from
Closed

Conversation

kikugie
Copy link

@kikugie kikugie commented Apr 30, 2024

ConfigScreenFactory factory in ModMenu is allowed to be null, in which case the entry is skipped. This behavior is intentional and allows mods to have optional dependencies on config libraries.
I.e. if you return null in getModConfigScreenFactory() it means "I would like to be compatible with modmenu, but can't rn".

This PR adds this check to the mod to replicate the behavior and fix a crash when a mod follows this pattern.

@MrCrayfish
Copy link
Owner

MrCrayfish commented Apr 30, 2024

Thanks for the info. I should have looked at the code more closely, however it seems the method docs doesn't mention this looking at it now. Should getProvidedConfigScreenFactories() be prevented from running if getModConfigScreenFactory() returns null?

@MrCrayfish
Copy link
Owner

The reason I followed non-null is because it's not documented. I have submitted an issue on Mod Menu repo (TerraformersMC/ModMenu#724) to clarify the problem. Once I get a response, I'll decide what happens.

@MrCrayfish
Copy link
Owner

Another point I bring up is that returning null on ConfigScreenFactory#create can be used to do exactly the same, since returning a null Screen will do nothing too. Regardless if you return null for the factory, it is still put into the ModMenu#configScreenFactories map.

@kikugie
Copy link
Author

kikugie commented May 10, 2024

Both approaches are valid in modmenu, but one causes a crash with this mod, so iono looks worth it to me

@MrCrayfish
Copy link
Owner

Fixed in 2b18b42

@MrCrayfish MrCrayfish closed this Nov 18, 2024
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.

2 participants