-
Notifications
You must be signed in to change notification settings - Fork 340
refactor(core): extract MessageBox to separate UI package #590
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
refactor(core): extract MessageBox to separate UI package #590
Conversation
Make core lightweight by extracting MessageBox with an abstraction layer: - Add Prompt abstraction in core for user dialogs (delegate-based) - Create new jengine.ui package for UI utilities - Move MessageBox from core to ui package (namespace: JEngine.UI) - Update Bootstrap to use Prompt.ShowDialogAsync instead of MessageBox - Add PromptInitializer.cs for easy UI integration Package structure after changes: - jengine.core: Independent hot update only, with Prompt abstraction - jengine.ui: Optional UI utilities (MessageBox) Users can customize prompt behavior by: 1. Using the default MessageBox via PromptInitializer 2. Implementing custom dialog providers This keeps core non-invasive (无侵入式) for hot updates while allowing UI customization through the optional ui package. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
View workflow run |
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 the MessageBox UI component out of the core JEngine framework into a separate optional jengine.ui package, introducing a Prompt abstraction layer to maintain loose coupling while allowing UI customization.
Changes:
- Introduces
Promptabstraction in core package for dialog operations via delegate pattern - Extracts
MessageBoxfromJEngine.Core.Miscto newjengine.uipackage underJEngine.UInamespace - Updates
Bootstrapto usePrompt.ShowDialogAsyncinstead of direct MessageBox calls - Adds comprehensive unit tests for MessageBox with Editor-only test hooks (
TestHandler,SimulateNoPrefab) - Updates CI/CD workflows to support building and testing the new UI package
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| UnityProject/Packages/packages-lock.json | Adds jengine.ui package dependency with UniTask |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/package.json | Defines new UI package metadata and dependencies |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/MessageBox.cs | Moved MessageBox from core to UI package with namespace change and test hooks |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/JEngine.UI.asmdef | Assembly definition for UI package runtime code |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/MessageBoxTests.cs | Comprehensive unit tests for MessageBox using test hooks |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/JEngine.UI.Editor.Tests.asmdef | Test assembly definition referencing NUnit and UniTask |
| UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs | New abstraction layer for dialogs using delegate pattern |
| UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Bootstrap.cs | Updated to use Prompt.ShowDialogAsync instead of MessageBox |
| UnityProject/Assets/Scripts/PromptInitializer.cs | Sample initializer showing how to register MessageBox as dialog provider |
| UnityProject/Assets/HotUpdate/Code/EntryPoint.cs | Updates namespace import from JEngine.Core.Misc to JEngine.UI |
| UnityProject/Assets/HotUpdate/Code/HotUpdate.Code.asmdef | Adds assembly references for JEngine.Util and JEngine.UI |
| .github/workflows/release.yml | Adds UI package to release workflow with version management |
| .github/workflows/pr-tests.yml | Updates test workflow to include UI package coverage |
Comments suppressed due to low confidence (2)
UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/MessageBox.cs:226
- The SimulateNoPrefab flag duplicates the prefab null check logic. When SimulateNoPrefab is true, it returns early with an error message that is identical to the real prefab null check at line 228-232. This creates code duplication and makes the test behavior different from the actual error path.
A cleaner approach would be to make SimulateNoPrefab affect the Prefab property getter (lines 91-104) to return null when simulating, rather than having a separate check. This would ensure tests exercise the exact same code path as production when the prefab is actually missing.
UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/MessageBox.cs:240
- The TestHandler is checked after prefab validation, which means tests will attempt to load the prefab from Resources even when using TestHandler. This creates unnecessary resource loading overhead in tests and may cause test failures if the prefab doesn't exist in the test environment.
The order should be reversed: check TestHandler first before attempting any prefab operations. This allows tests to completely bypass the UI infrastructure.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Outdated
Show resolved
Hide resolved
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Outdated
Show resolved
Hide resolved
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Outdated
Show resolved
Hide resolved
UnityProject/Packages/com.jasonxudeveloper.jengine.ui/package.json
Outdated
Show resolved
Hide resolved
- Make ShowDialogAsync thread-safe with volatile backing field and Interlocked.Exchange - Improve XML documentation for Prompt class with usage examples - Change default behavior to log error and return false (safer default) - Add namespace to PromptInitializer (JEngine) - Add TextMeshPro dependency to jengine.ui package.json - Remove JEngine.UI dependency from HotUpdate.Code (use Prompt abstraction instead) - Update EntryPoint.cs to use Prompt.ShowDialogAsync instead of MessageBox.Show Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
- Use null-coalescing in getter instead of field initializer - Remove namespace from PromptInitializer (user-level code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
HotUpdate code should not directly depend on utility packages. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
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.
Auto-approved: Copilot review found no issues and Unity Tests passed (or were skipped for non-code changes).
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.
Auto-approved: Copilot review found no issues and Unity Tests passed (or were skipped for non-code changes).
Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
New package should start at 0.0.0. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
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.
Auto-approved: Copilot review found no issues and Unity Tests passed (or were skipped for non-code changes).
- New packages should start at version 0.0.0 - Document naming convention and required files - Add ui scope to commit message conventions Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
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 26 out of 26 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Show resolved
Hide resolved
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Show resolved
Hide resolved
UnityProject/Packages/com.jasonxudeveloper.jengine.core/Runtime/Prompt.cs
Show resolved
Hide resolved
Unity Test Results✅ EditMode: All tests passed Unity Version: 2022.3.55f1 ✅ All tests passed! The PR is ready for review. View workflow run |
Split verbose CLAUDE.md (295 lines) into focused files: - .claude/rules/commit-conventions.md - .claude/rules/package-creation.md - .claude/rules/coding-patterns.md Main CLAUDE.md now 84 lines with essential info only. Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
- Add @imports to CLAUDE.md for .claude/rules/ files - Update copilot instructions to allow no-namespace code in Assets/Scripts/ - Add JEngine.UI namespace to instruction files Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: JasonXuDeveloper - 傑 <[email protected]>
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.
Auto-approved: Copilot review found no issues and Unity Tests passed (or were skipped for non-code changes).
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.
Auto-approved: Copilot review found no issues and Unity Tests passed (or were skipped for non-code changes).
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 31 out of 31 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Make core lightweight by extracting MessageBox with an abstraction layer, keeping the framework non-invasive (无侵入式) for hot updates while allowing UI customization.
Changes
Promptabstraction in core for user dialogs (delegate-based, thread-safe)jengine.uipackage for UI utilitiesMessageBoxfrom core to ui package (namespace:JEngine.UI)Bootstrapto usePrompt.ShowDialogAsyncinstead ofMessageBoxPromptInitializer.csthat registers MessageBox as defaultTestHandler,SimulateNoPrefab)Prompt.ShowDialogAsync(no direct UI/Util dependencies)Package Structure
jengine.corejengine.utiljengine.uiHow It Works
Prompt.ShowDialogAsyncmust be defined before Bootstrap starts. Using[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)]ensures this.Default behavior (this repo):
This repo includes
jengine.uipackage andAssets/Scripts/PromptInitializer.cswhich registers MessageBox by default:Custom implementation:
If you don't want to use our MessageBox, modify
PromptInitializer.cswith your own dialog logic:Without any initializer:
If
Prompt.ShowDialogAsyncis not set, Bootstrap logs errors and returnsfalse(safer default - operations requiring confirmation will not proceed).Breaking Changes
MessageBoxmoved fromJEngine.Core.MisctoJEngine.UITest plan
jengine.core- compiles without MessageBox dependencyjengine.ui- compiles with MessageBox🤖 Generated with Claude Code