refactor(ModuleManager): replace interface with concrete class

[keturn]

This removes an Interface that only has one implementation.

[Earlier, this PR removed ModuleManagerFactory as well, but I talked myself out of that in the comments below.]

A detail I'm noticing now that had passed me by earlier is that factory is only used by engine-test, not the engine itself.

I think the decisions I made in #4479 still apply, but maybe this explains part of the history about how redundant code ended up in there.

[keturn]

"remove interface" happens over two commits in hopes that would make it clearer to git that ModuleManagerImpl was moved to ModuleManager, as opposed to ModuleManager just adding a bunch of lines with no history.

will it actually work that way when it comes to annotation and conflict resolution? who knows

[keturn]

the previous branch for gestalt v7 (#4144) has no ModuleManager.loadModulesFromClasspath at all: https://github.com/MovingBlocks/Terasology/blob/f987e3b57c2db0f3d883c200e64befdb9d58b765/engine/src/main/java/org/terasology/engine/core/module/ModuleManagerImpl.java

in which case it probably is a good thing for engine-test to have that factory that provides its unittest module:

Terasology/engine-tests/src/main/java/org/terasology/testUtil/ModuleManagerFactory.java

Lines 25 to 29 in f987e3b

	Module unittestModule = moduleManager.getModuleFactory().createPackageModule("org.terasology.unittest");
	moduleManager.getRegistry().add(unittestModule);
	moduleManager.loadEnvironment(Sets.newHashSet(moduleManager.getRegistry().getLatestModuleVersion(new Name(
	"engine")), unittestModule), true);

[keturn]

I have no problem with loadModulesFromClasspath going away, especially as a thing that happens by default.

But don't MTE tests require it?

I say that because the terasology-module build puts the dependencies of a module on its classpath during tests.

To do away with that entirely, we'd have to have MTE tests use a build dependency on the cachedModules collection, which would be significant work. Not so much for CI, where we could always build cachedModules before tests, but for interactive use in the IDE where you run tests ad-hoc, that would take considerable doing.

but if it's only for MTE's benefit, then the loadModulesFromClasspath stuff could live in there.

that's all a tangent for v7 implementation. The relevant bit for this PR is that engine-test's ModuleFactory can stay; I'll do a force-push to this PR to reflect that.

[keturn]

I discovered something useful via GitHub history!

This commit effectively undoes #1665 (from 2015), so in reviewing this do take in to consideration what msteiger said about the advantages of having an Interface:

Yeah, I want to be able to load modules at runtime in WorldViewer, too. However, it needs to be done a little bit different. There is no module folder, for example.

Using interfaces instead of concrete classes gives a lot of flexibility in general, in particular if they are used as some kind of service in many different places (through CoreRegistry). Refactoring the access to module metadata could be seen as a general code cleanup process.

[keturn]

I took a look, and there are a lot of places that pull the ModuleManager out of the CoreRegistry or Context or whatever.

Many (but not all) of them do so only to get the current Environment from it, which makes me wonder if that points to a split between its registry and environment.

However, the only place that creates one of these is TerasologyEngine. So if we're adding layers of abstraction to this, we're doing it for the benefit of something that is not Engine.

I'm not familiar with WorldViewer but I guess I can imagine a thing that wants to inspect the state of the world without making everything tick would work like that?

Use of code from org.terasology.engine that doesn't use TerasologyEngine is not something I've given much thought to. I'm skeptical about how much of it is usable in that way if it hasn't been extracted to a subsystem or library first.

I'd be content to leave this Interface as-is if there is a use case for it.

[skaldarnar]

I think we can ignore the WorldViewer in the current situation. It was a pretty neat tool, but wasn't in use for some time now ... It would be nice to revive at some point in time, but the connection to CoreRegistry is more concerning than reassuring...

Maybe something we could put on the list after migration to gestalt v7, potentially as a GSOC project at some point...