-
Notifications
You must be signed in to change notification settings - Fork 28
Refactor Level Setup, Import and Rename forms + Refactor script generators #1120
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors level setup/import/rename flows by extracting logic from WinForms into dedicated services and helper classes (PRJ2 handling, script-line generation, game-version defaults), while also updating some shared API shapes to use read-only collections.
Changes:
- Replaced the old
LevelHandlingutility with focused helpers (Prj2Helper,LevelNameHelper,ScriptLineGenerator,GameVersionHelper) and updated call sites. - Introduced new ProjectMaster services for level setup/import/rename with options/result records and optional progress reporting.
- Updated UI entry points (
SectionLevelList, forms) to depend on the new services and adjusted namespaces/types (LevelCompileService,EngineVersionInfo, append-script-lines API).
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TombIDE/TombIDE/Forms/FormImportProject.cs | Switches PRJ2 discovery to Prj2Helper.GetValidFiles(...). |
| TombIDE/TombIDE.Shared/TombIDE.Shared.csproj | Sets LangVersion to 12 for Shared project. |
| TombIDE/TombIDE.Shared/SharedForms/FormLoading.cs | Uses Prj2Helper.UpdateGameSettings instead of removed LevelHandling. |
| TombIDE/TombIDE.Shared/SharedClasses/ScriptLineGenerator.cs | New script line generator replacing LevelHandling.GenerateScriptLines. |
| TombIDE/TombIDE.Shared/SharedClasses/LevelNameHelper.cs | New validation/sanitization helpers replacing name-related parts of LevelHandling. |
| TombIDE/TombIDE.Shared/SharedClasses/LevelHandling.cs | Removes legacy all-in-one helper. |
| TombIDE/TombIDE.Shared/SharedClasses/GameVersionHelper.cs | Centralizes game-version-specific defaults/capabilities for UI/services. |
| TombIDE/TombIDE.Shared/NewStructure/Prj2Helper.cs | Extends PRJ2 helper with valid-file enumeration + settings update logic. |
| TombIDE/TombIDE.Shared/IProgressReportingForm.cs | Adds progress-reporting interface for long-running imports. |
| TombIDE/TombIDE.Shared/IDE.cs | Changes append-script-lines event/API to IReadOnlyList<string>. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Setup/LevelSetupService.cs | New service that creates level folder + PRJ2 + optional script lines. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Setup/LevelSetupResult.cs | Result record for setup flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Setup/LevelSetupOptions.cs | Options record for setup flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Setup/ILevelSetupService.cs | Interface for level setup service. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Rename/LevelRenameState.cs | Captures UI checkbox/label state logic for rename form. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Rename/LevelRenameService.cs | New service handling rename logic + UI state computation. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Rename/LevelRenameResult.cs | Result record for rename flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Rename/LevelRenameOptions.cs | Options record for rename flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Rename/ILevelRenameService.cs | Interface for rename service. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Import/LevelImportService.cs | New service handling copy/keep import modes + PRJ2 updates + progress. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Import/LevelImportResult.cs | Result record for import flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Import/LevelImportOptions.cs | Options record for import flow. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Import/LevelImportMode.cs | Enum for import modes. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Import/ILevelImportService.cs | Interface for import service. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Compile/LevelCompileService.cs | Namespace refactor for compile service. |
| TombIDE/TombIDE.ProjectMaster/Services/Level/Compile/ILevelCompileService.cs | Namespace refactor for compile service interface. |
| TombIDE/TombIDE.ProjectMaster/Services/EngineVersion/IEngineVersionService.cs | Removes embedded EngineVersionInfo type (moved out). |
| TombIDE/TombIDE.ProjectMaster/Services/EngineVersion/EngineVersionService.cs | Refactors version-info creation to use new EngineVersionInfo record. |
| TombIDE/TombIDE.ProjectMaster/Services/EngineVersion/EngineVersionInfo.cs | New record type for engine version status. |
| TombIDE/TombIDE.ProjectMaster/Sections/SectionLevelList.cs | Wires new services into UI and switches script-lines type to read-only list. |
| TombIDE/TombIDE.ProjectMaster/LevelManager.cs | Constructs/injects new services into SectionLevelList. |
| TombIDE/TombIDE.ProjectMaster/Forms/FormRenameLevel.cs | Migrates rename logic/UI state to ILevelRenameService. |
| TombIDE/TombIDE.ProjectMaster/Forms/FormLevelSetup.cs | Migrates setup logic to ILevelSetupService and shared helpers. |
| TombIDE/TombIDE.ProjectMaster/Forms/FormImportLevel.cs | Migrates import logic to ILevelImportService and adds progress reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Can you please make sure the core features haven't changed with this refactor, and that all affected features still work as they should? |
|
@Nickelony I've opened a new pull request, #1121, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Nickelony <20436882+Nickelony@users.noreply.github.com>
Fix duplicate property declaration in ScriptGenerationResult record
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public IReadOnlyList<GeneratedScriptFile> FilesToCreate { get; init; } = []; | ||
|
|
||
| /// <summary> | ||
| /// Whether any script content was generated. | ||
| /// </summary> | ||
| public bool HasContent => GameFlowScript.Length > 0 || LanguageScript.Length > 0; |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScriptGenerationResult.HasContent ignores FilesToCreate, but multiple callers (e.g., ScriptEditor append events) use HasContent to decide whether to process the result at all. This can cause generated files (e.g., new level Lua files) to never be created when scripts are empty but FilesToCreate is not. Consider including FilesToCreate.Count > 0 in HasContent or introducing a separate property that reflects any output (scripts or files).
| var collectionNameLine = stringsEditor.Document.Lines.FirstOrDefault(line => regex.IsMatch(stringsEditor.Document.GetText(line).Replace(" ", string.Empty))); | ||
|
|
||
| string stringsVariableName = regex.Match(stringsEditor.Document.GetText(collectionNameLine)).Groups[1].Value; | ||
| if (collectionNameLine == null) | ||
| return; | ||
|
|
||
| regex = new Regex(@"local\s+" + Regex.Escape(stringsVariableName) + @"\s*="); | ||
| var stringsStartLine = stringsEditor.Document.Lines.FirstOrDefault(line => regex.IsMatch(stringsEditor.Document.GetText(line))); | ||
| string stringsVariableName = regex.Match(stringsEditor.Document.GetText(collectionNameLine)).Groups[1].Value; | ||
|
|
||
| if (stringsStartLine == null) | ||
| return; | ||
| regex = new Regex(@"local\s+" + Regex.Escape(stringsVariableName) + @"\s*="); |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The early return statements inside the language-file insertion block exit AppendScript without restoring/saving/closing the language tab and without creating result.FilesToCreate. This can leave the editor in an inconsistent state and skip creating generated files. Refactor to avoid returning from the method here (e.g., use a local flag / goto cleanup) so cleanup and file creation still run even when the language insertion can’t be performed.
|
|
||
| var openingBrackets = new Stack<char>(); | ||
| DocumentLine stopLine = null; | ||
| if (stringsStartLine == null) | ||
| return; | ||
|
|
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above: returning here aborts the rest of AppendScript, skipping tab cleanup and FilesToCreate creation. Prefer breaking out of the language insertion logic and continuing with the rest of the method instead of returning.
| if (stopLine == null || !stringsEditor.Document.GetText(stopLine).Trim().Equals("}")) | ||
| return; |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning here also aborts the remainder of AppendScript (including file creation and any editor/tab cleanup). Consider handling this as a non-fatal language-insertion failure and continuing to process the rest of the script generation result.
No description provided.