Conversation
|
Please update the target branch to |
| import java.util.concurrent.ExecutionException; | ||
|
|
||
| public record ExtractNeoforgeMdkStep(FilesService files, ZipService zip) implements CreationStep { | ||
| private static final String GRADLE_VERSION = "8.14.3"; |
There was a problem hiding this comment.
This cannot be correct? Having a hardcoded constant for the gradle version can't be right. If this is a temporary thing, a TODO needs to be added, and mention this somewhere in the PR.
| files.writeString(newMainClassPath, compilationUnit.toString(DEFAULT_PRINTER_CONFIGURATION)); | ||
|
|
||
| ParserConfiguration config = new ParserConfiguration(); | ||
| config.setLanguageLevel(ParserConfiguration.LanguageLevel.JAVA_17); |
There was a problem hiding this comment.
Once again, JAVA_17 shouldn't be hardcoded without some notice of a TODO. Things like this should be variable, if you're unable to do that, then a TOOD needs to be added, and ideally make an issue for it.
| package dev.railroadide.railroad.project.creation.step; | ||
|
|
||
| import dev.railroadide.core.project.ProjectContext; | ||
| import dev.railroadide.core.project.ProjectData; | ||
| import dev.railroadide.core.project.ProjectType; | ||
| import dev.railroadide.core.project.creation.CreationStep; | ||
| import dev.railroadide.core.project.creation.ProgressReporter; | ||
| import dev.railroadide.core.project.creation.service.FilesService; | ||
| import dev.railroadide.core.project.creation.service.HttpService; | ||
| import dev.railroadide.core.project.creation.service.TemplateEngineService; | ||
| import dev.railroadide.core.project.minecraft.MappingChannel; | ||
| import dev.railroadide.core.switchboard.pojo.MinecraftVersion; | ||
| import dev.railroadide.railroad.project.MappingChannelRegistry; | ||
| import dev.railroadide.railroad.project.ProjectTypeRegistry; | ||
| import dev.railroadide.railroad.project.creation.ProjectContextKeys; | ||
| import dev.railroadide.railroad.project.data.FabricProjectKeys; | ||
| import dev.railroadide.railroad.project.data.ForgeProjectKeys; | ||
| import dev.railroadide.railroad.project.data.MinecraftProjectKeys; | ||
| import groovy.lang.Binding; | ||
|
|
||
| import java.net.URI; | ||
| import java.nio.file.Path; | ||
| import java.util.HashMap; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
|
|
||
| public record UpdateForgeGradleFilesStep(FilesService files, HttpService http, TemplateEngineService templateEngine, | ||
| String branch, boolean includeSettingsGradle) implements CreationStep { | ||
| private static final String TEMPLATE_BUILD_GRADLE_URL = "https://raw.githubusercontent.com/Railroad-Team/Railroad/%s/templates/forge/%s/template_build.gradle"; | ||
|
|
||
| private static final String TEMPLATE_SETTINGS_GRADLE_URL = "https://raw.githubusercontent.com/Railroad-Team/Railroad/%s/templates/forge/%s/template_settings.gradle"; | ||
|
|
||
| @Override | ||
| public String id() { | ||
| return "railroad:update_gradle_files"; | ||
| } | ||
|
|
||
| @Override | ||
| public String translationKey() { | ||
| return "railroad.project.creation.task.update_gradle_files"; | ||
| } | ||
|
|
||
| @Override | ||
| public void run(ProjectContext ctx, ProgressReporter reporter) throws Exception { | ||
| updateBuildGradle(ctx, reporter); | ||
| if (includeSettingsGradle) | ||
| updateSettingsGradle(ctx, reporter); | ||
| } | ||
|
|
||
| private void updateBuildGradle(ProjectContext ctx, ProgressReporter reporter) throws Exception { | ||
| reporter.info("Downloading template build.gradle..."); | ||
|
|
||
| Path projectDir = ctx.projectDir(); | ||
| Path buildGradlePath = projectDir.resolve("build.gradle"); | ||
|
|
||
| MinecraftVersion mdkVersion = ctx.get(ProjectContextKeys.MDK_VERSION); | ||
| if (mdkVersion == null) | ||
| throw new IllegalStateException("MDK version not set in project context"); | ||
|
|
||
| String templateBuildGradleUrl = TEMPLATE_BUILD_GRADLE_URL.formatted(branch, mdkVersion.id().substring("1.".length())); | ||
| if (http.isNotFound(new URI(templateBuildGradleUrl))) { | ||
| MinecraftVersion minecraftVersion = ctx.data().get(MinecraftProjectKeys.MINECRAFT_VERSION, MinecraftVersion.class); | ||
| if (minecraftVersion == null) | ||
| throw new IllegalStateException("Minecraft version not set in project context"); | ||
|
|
||
| templateBuildGradleUrl = TEMPLATE_BUILD_GRADLE_URL.formatted(branch, minecraftVersion.id().substring("1.".length())); | ||
| if (http.isNotFound(new URI(templateBuildGradleUrl))) | ||
| throw new IllegalStateException("Template build.gradle not found for version " + mdkVersion.id() + " or " + minecraftVersion.id()); | ||
| } | ||
|
|
||
| Path templateBuildGradlePath = buildGradlePath.resolveSibling("template_build.gradle"); | ||
| http.download(new URI(templateBuildGradleUrl), templateBuildGradlePath); | ||
|
|
||
| reporter.info("Updating build.gradle..."); | ||
| String templateContent = files.readString(templateBuildGradlePath); | ||
| if (!templateContent.startsWith("// fileName: ")) | ||
| throw new IllegalStateException("Invalid template build.gradle file: missing fileName metadata"); | ||
|
|
||
| updateContent(ctx, projectDir, buildGradlePath, templateBuildGradlePath, templateContent); | ||
| } | ||
|
|
||
| private void updateSettingsGradle(ProjectContext ctx, ProgressReporter reporter) throws Exception { | ||
| reporter.info("Downloading template settings.gradle..."); | ||
|
|
||
| Path projectDir = ctx.projectDir(); | ||
| Path settingsGradlePath = projectDir.resolve("settings.gradle"); | ||
|
|
||
| MinecraftVersion mdkVersion = ctx.get(ProjectContextKeys.MDK_VERSION); | ||
| if (mdkVersion == null) | ||
| throw new IllegalStateException("MDK version not set in project context"); | ||
|
|
||
| String templateSettingsGradleUrl = TEMPLATE_SETTINGS_GRADLE_URL.formatted(branch, mdkVersion.id().substring("1.".length())); | ||
| if (http.isNotFound(new URI(templateSettingsGradleUrl))) { | ||
| MinecraftVersion minecraftVersion = ctx.data().get(MinecraftProjectKeys.MINECRAFT_VERSION, MinecraftVersion.class); | ||
| if (minecraftVersion == null) | ||
| throw new IllegalStateException("Minecraft version not set in project context"); | ||
|
|
||
| templateSettingsGradleUrl = TEMPLATE_SETTINGS_GRADLE_URL.formatted(branch, minecraftVersion.id().substring("1.".length())); | ||
| if (http.isNotFound(new URI(templateSettingsGradleUrl))) | ||
| throw new IllegalStateException("Template settings.gradle not found for version " + mdkVersion.id() + " or " + minecraftVersion.id()); | ||
| } | ||
|
|
||
| Path templateSettingsGradlePath = settingsGradlePath.resolveSibling("template_settings.gradle"); | ||
| http.download(new URI(templateSettingsGradleUrl), templateSettingsGradlePath); | ||
|
|
||
| reporter.info("Updating settings.gradle..."); | ||
| String templateContent = files.readString(templateSettingsGradlePath); | ||
| if (!templateContent.startsWith("// fileName: ")) | ||
| throw new IllegalStateException("Invalid template settings.gradle file: missing fileName metadata"); | ||
|
|
||
| updateContent(ctx, projectDir, settingsGradlePath, templateSettingsGradlePath, templateContent); | ||
| } | ||
|
|
||
| private void updateContent(ProjectContext ctx, Path projectDir, Path settingsGradlePath, Path templateSettingsGradlePath, String templateContent) throws Exception { | ||
| Map<String, Object> args = createGradleBindings(ctx.data()); | ||
| var binding = new Binding(args); | ||
| binding.setVariable("defaultName", projectDir.relativize(settingsGradlePath.toAbsolutePath()).toString()); | ||
|
|
||
| String updatedContent = templateEngine.apply(templateContent, args); | ||
| files.writeString(settingsGradlePath, updatedContent); | ||
| files.delete(templateSettingsGradlePath); | ||
| } | ||
|
|
||
| private static Map<String, Object> createGradleBindings(ProjectData data) { | ||
| ProjectType projectType = data.get(ProjectData.DefaultKeys.TYPE, ProjectType.class); | ||
| if (projectType == null) | ||
| throw new IllegalStateException("Project type not set in project data"); | ||
|
|
||
| MappingChannel defaultChannel = getDefaultMappingChannel(projectType); | ||
|
|
||
| final Map<String, Object> args = new HashMap<>(); | ||
| args.put("mappings", Map.of( | ||
| "channel", data.getOrDefault(MinecraftProjectKeys.MAPPING_CHANNEL, defaultChannel, MappingChannel.class).id().toLowerCase(Locale.ROOT), | ||
| "version", data.getAsString(MinecraftProjectKeys.MAPPING_VERSION) | ||
| )); | ||
|
|
||
| Map<String, Object> props = new HashMap<>(); | ||
| if (projectType == ProjectTypeRegistry.FABRIC) { | ||
| props.putAll(Map.of( | ||
| "splitSourceSets", data.getAsBoolean(FabricProjectKeys.SPLIT_SOURCES), | ||
| "includeFabricApi", data.contains(FabricProjectKeys.FABRIC_API_VERSION), | ||
| "useAccessWidener", data.getAsBoolean(FabricProjectKeys.USE_ACCESS_WIDENER), | ||
| "accessWidenerPath", data.contains(FabricProjectKeys.ACCESS_WIDENER_PATH) ? | ||
| data.getAsString(FabricProjectKeys.ACCESS_WIDENER_PATH) : | ||
| data.getAsString(MinecraftProjectKeys.MOD_ID) + ".accesswidener", | ||
| "modId", data.getAsString(MinecraftProjectKeys.MOD_ID) | ||
| )); | ||
| } else if (projectType == ProjectTypeRegistry.FORGE || projectType == ProjectTypeRegistry.NEOFORGE) { | ||
| props.putAll(Map.of( | ||
| "useMixins", data.getAsBoolean(ForgeProjectKeys.USE_MIXINS), | ||
| "useAccessTransformer", data.getAsBoolean(ForgeProjectKeys.USE_ACCESS_TRANSFORMER), | ||
| "genRunFolders", data.getAsBoolean(ForgeProjectKeys.GEN_RUN_FOLDERS) | ||
| )); | ||
| } else { | ||
| throw new IllegalStateException("Unsupported project type: " + projectType); | ||
| } | ||
|
|
||
| args.put("props", props); | ||
| return args; | ||
| } | ||
|
|
||
| public static MappingChannel getDefaultMappingChannel(ProjectType projectType) { | ||
| if (projectType.equals(ProjectTypeRegistry.FABRIC)) { | ||
| return MappingChannelRegistry.YARN; | ||
| } else if (projectType.equals(ProjectTypeRegistry.FORGE)) { | ||
| return MappingChannelRegistry.MOJMAP; | ||
| } else if (projectType.equals(ProjectTypeRegistry.NEOFORGE)) { | ||
| return MappingChannelRegistry.PARCHMENT; | ||
| } | ||
|
|
||
| throw new IllegalStateException("Unsupported project type: " + projectType); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this class necessary? As far as I can tell its just an exact copy of UpdateFabricGradleFilesStep.
| .addStep("project_details", this::createProjectDetailsStep) // name, loc | ||
| .addStep("maven_coordinates", this::createMavenCoordinatesStep) // groupid, artifact id, version |
There was a problem hiding this comment.
| .addStep("project_details", this::createProjectDetailsStep) // name, loc | |
| .addStep("maven_coordinates", this::createMavenCoordinatesStep) // groupid, artifact id, version | |
| .addStep("project_details", this::createProjectDetailsStep) | |
| .addStep("maven_coordinates", this::createMavenCoordinatesStep) |
| .addStep("minecraft_version", this::createMinecraftVersionStep) | ||
| .addStep("forge_version", this::createForgeVersionStep) | ||
| .addStep("mapping_channel", this::createMappingChannelStep) | ||
| .addStep("mapping_channel", this::createMappingChannelStep) // parchment or mojmaps |
There was a problem hiding this comment.
| .addStep("mapping_channel", this::createMappingChannelStep) // parchment or mojmaps | |
| .addStep("mapping_channel", this::createMappingChannelStep) |
|
|
||
| return null; | ||
| }) | ||
| .defaultValue(() -> MappingChannelRegistry.PARCHMENT) |
There was a problem hiding this comment.
| .defaultValue(() -> MappingChannelRegistry.PARCHMENT) | |
| .defaultValue(() -> { | |
| if (availableChannels.contains(MappingChannelRegistry.PARCHMENT)) | |
| return MappingChannelRegistry.PARCHMENT; | |
| if (!availableChannels.isEmpty()) | |
| return availableChannels.getFirst(); | |
| return null; | |
| }) |
There was a problem hiding this comment.
Why is this still a thing?
| import com.github.javaparser.ParserConfiguration; | ||
| import com.github.javaparser.StaticJavaParser; |
There was a problem hiding this comment.
| import com.github.javaparser.ParserConfiguration; | |
| import com.github.javaparser.StaticJavaParser; |
There was a problem hiding this comment.
I understand why you did this, but this very much limits us to only having very specific projects and layouts, where we might not want them in the future, or plugins may not want them. So a better option maybe is to have a bit of an inheritance tree (i.e. Onboarding -> MavenOnboarding > MinecraftOnboarding)
| String[] p = id.split("\\."); | ||
| String[] t = {"1", "20", "4"}; | ||
| for (int i = 0; i < Math.max(p.length, t.length); i++) { | ||
| int a = i < p.length ? Integer.parseInt(p[i]) : 0; | ||
| int b = i < t.length ? Integer.parseInt(t[i]) : 0; | ||
| if (a > b) return true; | ||
| if (a < b) return false; | ||
| } | ||
| return true; | ||
| }) | ||
| .sorted((a, b) -> { | ||
| String[] pa = a.id().split("\\."); | ||
| String[] pb = b.id().split("\\."); | ||
| for (int i = 0; i < Math.max(pa.length, pb.length); i++) { | ||
| int va = i < pa.length ? Integer.parseInt(pa[i]) : 0; | ||
| int vb = i < pb.length ? Integer.parseInt(pb[i]) : 0; |
There was a problem hiding this comment.
Not a fan of these variable names. Please use actual names.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the project onboarding flow by introducing a base Onboarding class to consolidate common logic across different mod loader implementations (Forge, NeoForge, Fabric). Key changes include:
- Created abstract
Onboardingbase class with shared step creation methods and utilities - Refactored
ForgeProjectOnboarding,NeoforgeProjectOnboarding, andFabricProjectOnboardingto extend the base class - Moved
OnboardingFlowBuilderintoOnboardingFlowas a nested static class with automatic transition generation - Renamed
UpdateGradleFilesSteptoUpdateFabricGradleFilesStepand createdUpdateForgeGradleFilesStep - Added separate NeoForge MDK download/extraction steps
- Updated gradle properties handling to differentiate between Forge and NeoForge
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| Onboarding.java | New abstract base class containing shared step creation methods and utility functions |
| OldNeoforgeProjectOnboarding.java | Preserved old implementation for reference |
| NeoforgeProjectOnboarding.java | Refactored to extend base Onboarding class, simplified version filtering |
| ForgeProjectOnboarding.java | Refactored to extend base Onboarding class, simplified version filtering |
| FabricProjectOnboarding.java | Refactored to extend base Onboarding class, removed duplicate methods |
| OnboardingFlowBuilder.java | Deleted - moved into OnboardingFlow as nested class |
| OnboardingFlow.java | Added nested OnboardingFlowBuilder with automatic transition generation |
| UpdateGradlePropertiesStep.java | Updated to differentiate Forge and NeoForge gradle properties handling |
| UpdateForgeModsTomlStep.java | Enhanced to search multiple locations for mods.toml/neoforge.mods.toml |
| UpdateForgeGradleFilesStep.java | New step for Forge-specific gradle file updates |
| UpdateFabricGradleFilesStep.java | Renamed from UpdateGradleFilesStep for Fabric-specific logic |
| RenameClassesStep.java | Added JavaParser configuration for Java 17 |
| ExtractNeoforgeMdkStep.java | New step for extracting NeoForge MDK |
| ExtractForgeMdkStep.java | Added MDK version resolution logic |
| DownloadNeoforgeMdkStep.java | New step for downloading NeoForge MDK |
| DefaultProjectCreationPipelineService.java | Updated to use renamed steps and separate Forge/NeoForge pipelines |
| Railroad.java | Added JavaParser imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| long epochSecond = candidate.releaseTime().toEpochSecond(ZoneOffset.UTC); | ||
| long candidateDiff = Math.abs(epochSecond - releaseTime); | ||
| long closestDiff = Math.abs(epochSecond - releaseTime); |
There was a problem hiding this comment.
The calculation for closestDiff is using epochSecond instead of closest.releaseTime().toEpochSecond(ZoneOffset.UTC). This means closestDiff will always equal candidateDiff when comparing against the same release time, making the comparison logic incorrect and preventing the algorithm from finding the actual closest release.
| long closestDiff = Math.abs(epochSecond - releaseTime); | |
| long closestDiff = Math.abs(closest.releaseTime().toEpochSecond(ZoneOffset.UTC) - releaseTime); |
| try { | ||
| CompletableFuture<List<String>> versionsFuture = SwitchboardRepositories.NEOFORGE.getVersionsFor(mcVersion.id()); | ||
| List<String> versions = versionsFuture.get(); | ||
| availableVersions.clear(); | ||
| availableVersions.addAll(versions); | ||
| ctx.markForRefresh(ForgeProjectKeys.FORGE_VERSION); | ||
| } catch (ExecutionException | InterruptedException exception) { | ||
| Railroad.LOGGER.error("Failed to fetch Neoforge versions for Minecraft {}", mcVersion.id(), exception); | ||
| } |
There was a problem hiding this comment.
The CompletableFuture.get() call blocks the UI thread synchronously. This should be handled asynchronously using Platform.runLater() in the whenComplete callback, similar to the pattern used in OldNeoforgeProjectOnboarding.java (lines 331-342).
| try { | |
| CompletableFuture<List<String>> versionsFuture = SwitchboardRepositories.NEOFORGE.getVersionsFor(mcVersion.id()); | |
| List<String> versions = versionsFuture.get(); | |
| availableVersions.clear(); | |
| availableVersions.addAll(versions); | |
| ctx.markForRefresh(ForgeProjectKeys.FORGE_VERSION); | |
| } catch (ExecutionException | InterruptedException exception) { | |
| Railroad.LOGGER.error("Failed to fetch Neoforge versions for Minecraft {}", mcVersion.id(), exception); | |
| } | |
| CompletableFuture<List<String>> versionsFuture = SwitchboardRepositories.NEOFORGE.getVersionsFor(mcVersion.id()); | |
| versionsFuture.whenComplete((versions, throwable) -> { | |
| Platform.runLater(() -> { | |
| if (throwable != null) { | |
| Railroad.LOGGER.error("Failed to fetch Neoforge versions for Minecraft {}", mcVersion.id(), throwable); | |
| } else { | |
| availableVersions.clear(); | |
| availableVersions.addAll(versions); | |
| ctx.markForRefresh(ForgeProjectKeys.FORGE_VERSION); | |
| } | |
| }); | |
| }); |
| try { | ||
| CompletableFuture<List<String>> versionsFuture = SwitchboardRepositories.FORGE.getVersionsFor(mcVersion.id()); | ||
| List<String> versions = versionsFuture.get(); | ||
| availableVersions.clear(); | ||
| availableVersions.addAll(versions); | ||
| ctx.markForRefresh(ForgeProjectKeys.FORGE_VERSION); | ||
| } catch (ExecutionException | InterruptedException exception) { | ||
| Railroad.LOGGER.error("Failed to fetch Forge versions for Minecraft {}", mcVersion.id(), exception); | ||
| } |
There was a problem hiding this comment.
The CompletableFuture.get() call blocks the UI thread synchronously. This should be handled asynchronously using Platform.runLater() in the whenComplete callback to avoid freezing the UI.
| } catch (InterruptedException exception) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
After re-interrupting the thread, the exception is wrapped and rethrown as an IllegalStateException. However, the interrupt status should be preserved by either propagating the InterruptedException or re-checking the interrupt status after catching it to ensure proper interrupt handling.
| applyToNode.accept(currentNode); | ||
| } | ||
|
|
||
| component.componentProperty().addListener((observable, oldValue, newValue) -> { |
There was a problem hiding this comment.
The parameter 'observable' is never used.
| component.componentProperty().addListener((observable, oldValue, newValue) -> { | |
| component.componentProperty().addListener((_observable, oldValue, newValue) -> { |
| Path projectDir = ctx.projectDir(); | ||
| Path mdkZip = projectDir.resolve("neoforge-mdk.zip"); | ||
|
|
||
| String repoBase = "https://github.com/NeoForgeMDKs/MDK-NeoForge-" + minecraftVersion.id(); |
There was a problem hiding this comment.
Variable 'String repoBase' is never read.
| String repoBase = "https://github.com/NeoForgeMDKs/MDK-NeoForge-" + minecraftVersion.id(); |
| @@ -94,7 +82,7 @@ public void start(Scene scene) { | |||
| process.run(scene); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This method overrides Onboarding.onFinish; it is advisable to add an Override annotation.
| @Override |
| public class FabricProjectOnboarding { | ||
| public class FabricProjectOnboarding extends Onboarding { | ||
| private final ExecutorService executor = Executors.newFixedThreadPool(4); | ||
|
|
There was a problem hiding this comment.
This method overrides Onboarding.start; it is advisable to add an Override annotation.
| @Override |
|
|
||
| public class ForgeProjectOnboarding extends Onboarding { | ||
| private final ExecutorService executor = Executors.newFixedThreadPool(4); | ||
|
|
There was a problem hiding this comment.
This method overrides Onboarding.start; it is advisable to add an Override annotation.
| @Override |
|
|
||
| public class NeoforgeProjectOnboarding extends Onboarding { | ||
| private final ExecutorService executor = Executors.newFixedThreadPool(4); | ||
|
|
There was a problem hiding this comment.
This method overrides Onboarding.start; it is advisable to add an Override annotation.
| @Override |
Forge onboarding tested for version 1.20.4,
Neoforge throws error when building but should work once neoforge templates are made