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

Look in the SERVICE layer for ImmediateWindowProvider implementations #92

Merged

Conversation

FiniteReality
Copy link
Member

This change was discussed in https://canary.discord.com/channels/313125603924639766/852298000042164244/1210370060737314818

There's a few ways to actually implement this, but I chose the most obvious. One behavioural change from this approach is that we no longer load ImmediateWindowProvider implementations from unnamed modules; if we're moving towards more use of modules, this seems like a sensible trade-off to me, and shouldn't pose a challenge for most mods as we build modules for them on the fly already.

Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

We also need to load jars that provide the service onto the SERVICE layer. Look at the two implementations of TransformerDiscoverySomething.

@FiniteReality
Copy link
Member Author

FiniteReality commented Feb 23, 2024

We also need to load jars that provide the service onto the SERVICE layer. Look at the two implementations of TransformerDiscoverySomething.

Is this referring to the SERVICES set in ModDirTransformerDiscoverer? I was under the impression that anything providing a custom immediate window provider would likely also be providing an ITransformationService or similar - I'm definitely doing that myself.

EDIT: If this is the case, there's an additional question I have; currently the SERVICES set in ModDirTransformerDiscoverer has:

  • cpw.mods.modlauncher.api.ITransformationService
  • net.neoforged.neoforgespi.locating.IModLocator
  • net.neoforged.neoforgespi.locating.IDependencyLocator

However, the ClasspathTransformerDiscoverer (presumably the other implementation you were referring to) only has ITransformationService and IModLocator - should I also enhance it with IDependencyLocator, or is there a reason why the two don't match?

@Technici4n
Copy link
Member

is there a reason why the two don't match

No idea! Looks like an oversight. We should deduplicate the set to avoid such mismatches in the future.

- Throws if the SERVICE layer was not built instead of using a default layer
- Extracts the discoverer service type constants to a new shared class and
  adds the ImmediateWindowProvider to this list.
@FiniteReality
Copy link
Member Author

I've implemented the requested changes, and also extracted the service set into a constants class that gets shared between the two discoverers, which I've added the ImmediateWindowProvider type to. I think that at some point in the future we should have a breaking change to move it into the SPI project, to be more consistent with the other service types.

@sciwhiz12 sciwhiz12 requested a review from Technici4n February 24, 2024 09:36
@sciwhiz12 sciwhiz12 added the enhancement New feature or request label Feb 24, 2024
@Technici4n
Copy link
Member

Looks good, will need to test this somehow however 😅

This was discussed in the NeoForge Discord
Copy link
Member

@embeddedt embeddedt left a comment

Choose a reason for hiding this comment

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

Simple and functional.

*/
void crash(String message);
}
@Deprecated(since = "2.0.16", forRemoval = true)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think I got this version number correct, but I could be wrong. I figured we should keep the interface around for binary compatibility at least, but I can remove it if it's deemed unnecessary.

@FiniteReality FiniteReality force-pushed the feature/service-layer-earlywindow branch from beb9dda to bd6ae49 Compare February 24, 2024 18:02
@Technici4n
Copy link
Member

Looks nice.

@marchermans
Copy link
Contributor

Looks nice.

I take this as an approval of the changes, and assumed that tech was unable to provide a proper approval due to external circumstances. Merging.

@marchermans marchermans dismissed Technici4n’s stale review February 25, 2024 09:39

Approved via comment

@marchermans marchermans merged commit d492af5 into neoforged:main Feb 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants