Update for new mixin #468
Update for new mixin #468
Conversation
| dataMap.put(FabricData.KEY_COMPATIBILITY, getMixinCompat(mod)); | ||
|
|
||
| for (String config : configs) { | ||
| addConfigurationFabric.invoke(null, config, dataMap); |
There was a problem hiding this comment.
Is reusing this map not an issue given the Mixin PR doesn't seem to clone it either? It's also not cleared after use but I suppose it overwrites everything it contains so it doesn't matter
There was a problem hiding this comment.
The Mixin PR actually "deserializes" the map directly
There was a problem hiding this comment.
So it does, thought FabricData#attach was keeping the map too but it's actually just the FabricData object
| getMixinConfigs(loader, side).forEach(FabricMixinBootstrap::addConfiguration); | ||
|
|
||
| try { | ||
| Method addConfigurationFabric = Mixins.class.getMethod("addConfigurationFabric", String.class, Map.class); |
There was a problem hiding this comment.
Could do this calling the method directly then catching NoSuchMethodError?
There was a problem hiding this comment.
I don't want to risk it, AFAIK the JVM spec doesn't guarantee that it'd be safe and this is good enough.
| ctor.setAccessible(true); | ||
| mixinTransformer = ctor.newInstance(); | ||
| } catch (ReflectiveOperationException e) { | ||
| throw new IllegalStateException("mixin transformer unavailable?"); |
There was a problem hiding this comment.
Might as well pass the original exception along too
There was a problem hiding this comment.
the exception is meant for the original fault (regular mixin interop failing), exceptions from this legacy support hack aren't meaningful outside Mixin 0.8-0.8.2
There was a problem hiding this comment.
I mean if it does throw a ReflectiveOperationException and you're not using Mixin 0.8.3+ presumably you'd want to know why rather than just getting a IllegalStateException without a cause
There was a problem hiding this comment.
There is no better cause really, i updated the commit with a few more comments.
sfPlayer1
force-pushed
the
mixinUpdate
branch
4 times, most recently
from
75a1584
to
0ecc1c3
Compare
|
|
||
| // default to old fabric-mixin | ||
|
|
||
| return FabricUtil.COMPATIBILITY_0_9_2; |
There was a problem hiding this comment.
In dev this should latest when a classpath mod, but fallback to to older version when loading mods from the mods dir.
There was a problem hiding this comment.
This might cause issues with outdated deps though... not sure there is a way to handle all the cases here.
sfPlayer1
force-pushed
the
mixinUpdate
branch
4 times, most recently
from
1363b8b
to
912d062
Compare
|
|
||
| for (Config config : Mixins.getConfigs()) { | ||
| ModContainerImpl mod = configToModMap.get(config.getName()); | ||
| if (mod == null) continue; |
There was a problem hiding this comment.
Whats the reason for this loop?
There was a problem hiding this comment.
I don't patch config addition in Mixin anymore, instead I add the config files normally, then pull the config objects back out, reassociate them with mods and add fabric specific data to them.
| import java.util.jar.Manifest; | ||
|
|
||
| public final class ManifestUtil { | ||
| public static final Name NAME_MAPPING_NAMESPACE = new Name("Fabric-Mapping-Namespace"); |
There was a problem hiding this comment.
Are these constants used now?
There was a problem hiding this comment.
hmm the whole class is unused for now..
There was a problem hiding this comment.
The file is actually still used through a refactor (moved Knot's manifest reading to it too), but now without the constants.
This makes Mixin act as if it was the oldest version required by the mod through its Loader dependency, to the extent implemented. Mods need to declare and bump their loader dependency properly to benefit from the latest enhancements in Mixin. Also remove now unused constants from ManifestUtil
This is still pending successful Mixin testing and gradle+installer dep updates