Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Conversation

@rikka0w0
Copy link
Contributor

@rikka0w0 rikka0w0 commented Jun 16, 2020

FMLModContainer and ModContainer:
[API] Add getMod and acceptEvent methods
[Internal] setParent and getParent: associated Fabric ModContainer
getter and setters

ModList:
Add most helper functions present in Forge's ModList, these are
frequenty used by mods

ModLoader(class):
A class may used by mods to post event on the FML event bus

TODO:
the ModContainer.setMod(Object) is not called anywhere in the code, I don't know how the forge mod main class instance is created. This is the only problem.

FMLModContainer and ModContainer:
 [API] Add getMod and acceptEvent methods
 [Internal] setParent and getParent: associated Fabric ModContainer
getter and setters

ModList:
 Add most helper functions present in Forge's ModList, these are
frequenty used by mods

ModLoader(class):
 A class may used by mods to post event on the FML event bus
@rikka0w0 rikka0w0 marked this pull request as ready for review June 16, 2020 19:27
@rikka0w0 rikka0w0 requested a review from coderbot16 June 19, 2020 10:53
@TheGlitch76 TheGlitch76 self-requested a review June 19, 2020 20:04
Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

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

Looks good except for the getMod and setMod, which should be removed if they're just going to cause a mysterious NPE later.


@Override
public Object getMod() {
return this.instance;
Copy link
Member

Choose a reason for hiding this comment

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

this seems to always be null, it might actually be better to drop them until the setter is actually called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we just comment getMod() and setMod() for now? Impl setMod needs some changes in the patcher as well, the ForgeInitializer.onForgeInitialize should return the instance of the Forge mod class, so that we can record it in its ModContainer.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, just comment them out for now and the change that needs Patcher can be a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we leave getMod() and setMod() here? Just add TODOs to both of them and throw a warning when they are called? Other methods in this pr require getMod(), simply remove getMod() needs more work.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. Make sure you open an issue on Patcher about it

@rikka0w0 rikka0w0 requested a review from TheGlitch76 June 21, 2020 19:29
Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

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

i'd like @coderbot16's review on this one because this part of the codebase is his territory, but looks good to me.

@TheGlitch76 TheGlitch76 added the finished This PR is close to being merged label Jun 23, 2020
@TheGlitch76 TheGlitch76 merged commit 3e8f584 into PatchworkMC:master Jun 30, 2020
@rikka0w0 rikka0w0 deleted the feature/fml-modlist branch July 5, 2020 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

finished This PR is close to being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants