Skip to content

Commit e877460

Browse files
fix: address code review feedback
- Fix Task.CompletedTask -> UniTask.CompletedTask - Fix AsUniTask to delegate to awaiter (prevent double pool return) - Fix Reset to return execution contexts to pool before clearing - Fix MessageBox test hooks to use direct index access (avoid foreach) - Add comment explaining PlayerLoop single-threading in parallel test Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
1 parent fcc2208 commit e877460

4 files changed

Lines changed: 25 additions & 39 deletions

File tree

UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/MessageBox.cs

Lines changed: 5 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -143,16 +143,8 @@ internal static bool TestSimulateButtonClick(bool clickOk)
143143
{
144144
if (ActiveMessageBoxes.Count == 0) return false;
145145

146-
// Get the most recent message box (any will do for testing)
147-
MessageBox target = null;
148-
foreach (var box in ActiveMessageBoxes)
149-
{
150-
target = box;
151-
break;
152-
}
153-
154-
if (target == null) return false;
155-
146+
// Get the first message box (any will do for testing)
147+
var target = ActiveMessageBoxes[0];
156148
target.HandleEvent(clickOk);
157149
return true;
158150
}
@@ -165,14 +157,8 @@ internal static (bool okVisible, bool noVisible)? TestGetButtonVisibility()
165157
{
166158
if (ActiveMessageBoxes.Count == 0) return null;
167159

168-
MessageBox target = null;
169-
foreach (var box in ActiveMessageBoxes)
170-
{
171-
target = box;
172-
break;
173-
}
174-
175-
if (target == null || target._buttonOk == null || target._buttonNo == null)
160+
var target = ActiveMessageBoxes[0];
161+
if (target._buttonOk == null || target._buttonNo == null)
176162
return null;
177163

178164
return (target._buttonOk.gameObject.activeSelf, target._buttonNo.gameObject.activeSelf);
@@ -186,15 +172,7 @@ internal static (string title, string content, string okText, string noText)? Te
186172
{
187173
if (ActiveMessageBoxes.Count == 0) return null;
188174

189-
MessageBox target = null;
190-
foreach (var box in ActiveMessageBoxes)
191-
{
192-
target = box;
193-
break;
194-
}
195-
196-
if (target == null) return null;
197-
175+
var target = ActiveMessageBoxes[0];
198176
return (
199177
target._title?.text,
200178
target._content?.text,

UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JAction.cs

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -910,8 +910,20 @@ public JAction Reset(bool force = false)
910910
_onCancelDelegate = null;
911911
_onCancelState?.Return();
912912
_onCancelState = null;
913+
914+
// Return active execution contexts to pool before clearing
915+
for (int i = 0; i < _activeContexts.Count; i++)
916+
{
917+
JActionExecutionContext.Return(_activeContexts[i]);
918+
}
913919
_activeContexts.Clear();
914-
_syncContext = null;
920+
921+
// Return sync execution context to pool
922+
if (_syncContext != null)
923+
{
924+
JActionExecutionContext.Return(_syncContext);
925+
_syncContext = null;
926+
}
915927

916928
return this;
917929
}

UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionExecutionHandle.cs

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -87,16 +87,9 @@ public void Cancel()
8787
/// </summary>
8888
public async UniTask<JActionExecution> AsUniTask()
8989
{
90-
await new JActionAwaitable(_context);
91-
_action?.RemoveActiveContext(_context);
92-
93-
// Capture cancelled state before returning context to pool
94-
bool cancelled = _context?.Cancelled ?? false;
95-
96-
// Return context to pool
97-
JActionExecutionContext.Return(_context);
98-
99-
return new JActionExecution(_action, cancelled);
90+
// Delegate to the handle's awaiter so that completion and cleanup
91+
// are managed in a single, centralized location.
92+
return await this;
10093
}
10194

10295
/// <summary>

UnityProject/Packages/com.jasonxudeveloper.jengine.util/Tests/Runtime/JActionRuntimeTests.cs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,6 +488,9 @@ public IEnumerator Do_AsyncFuncWithState_PassesState()
488488
[UnityTest]
489489
public IEnumerator Parallel_AllowsConcurrentExecution()
490490
{
491+
// Note: This test is safe despite shared variables because Unity's PlayerLoop
492+
// executes on a single thread. The increment/decrement operations are not
493+
// racing with each other - they execute sequentially within the same frame.
491494
int concurrentCount = 0;
492495
int maxConcurrent = 0;
493496

@@ -580,7 +583,7 @@ public IEnumerator Parallel_Property_ReflectsState()
580583
action1.Dispose();
581584
action2.Dispose();
582585

583-
await Task.CompletedTask;
586+
await UniTask.CompletedTask;
584587
});
585588
}
586589

0 commit comments

Comments
 (0)