Release 0.8.0 #16
Release 0.8.0 #16
Conversation
…er* to support presenters that open/close with a delay - Added new *DelayUiPresenter* to interact with *PresenterDelayerBase* implementations and allow presenters to open/close with a delay - Improved performance of *UiService* ***Changed**: - Removed *AnimatedUiPresenter*. Use the new *DelayUiPresenter* with one of the *PresenterDelayerBase* implementations - Removed *UiCloseActivePresenter* and *UiCloseActivePresenterData*. Use the new *DelayUiPresenter* with one of the *PresenterDelayerBase* implementations - Removed the dependency of *UiPresenter* from Canvas. Allowing different structures of UI Unity project hierarchy to work with the *UiService* - Removed all Get and Has methods from *IUiService*. Replaced with IReadOnlyDictionaries for all the collections being requested from the service - Changed all OpenUi methods to be async. This guarantees the expected behaviour that will always load the Ui first before opening - Changed all CloseUi methods to be synchronous. Closing an Ui will now always be atomic. To get the close delay, you can request directly from the *DelayUiPresenter* - Changed *IUiAssetLoader* to unify the prefab instantiation into a single call. This simplefies the method so the caller doesn't have to worry about synchronous or async behaviour - Changed the *UiConfig* to know contain the information of the *UiPresenter* if is loaded sync or async
|
Warning Rate limit exceeded@CoderGamester has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce version 0.8.0, featuring new classes for delayed UI actions, including Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (17)
Runtime/Delayers/AnimationDelayer.cs (1)
3-11: Consider improving documentation and namespace structure.
The class documentation could be enhanced with:
- Example usage
- Parameter requirements (e.g., required Animation component setup)
- Interaction with PresenterDelayerBase
Consider this documentation enhancement:
/// <inheritdoc />
/// <remarks>
/// Allows this Presenter to have an intro and outro animation when opened and closed to provide feedback and joy for players.
+/// <example>
+/// Usage:
+/// 1. Attach to a GameObject with an Animation component
+/// 2. Assign intro/outro AnimationClips
+/// 3. Configure in your DelayUiPresenter
+/// </example>
+/// <requires>
+/// - Animation component on the same GameObject
+/// - Configured AnimationClips for intro and outro
+/// </requires>
/// </remarks>Runtime/UiPresenter.cs (2)
8-14: Consider adding validation methods to the interface.
While the marker interface pattern is valid, consider whether adding validation methods like IsDataValid() would provide more value than a pure marker interface. This could help enforce data consistency across different presenter implementations.
86-89: LGTM! Consider documenting struct constraint rationale.
The implementation is correct. Consider adding XML documentation explaining why the struct constraint is important for the presenter data pattern.
Runtime/UiConfigs.cs (2)
19-19: Add documentation and consider default value for LoadSynchronously.
While the new field aligns with the PR objectives for async/sync loading configuration, consider:
- Adding XML documentation to explain its purpose and impact
- Specifying a default value to ensure consistent behavior
+/// <summary>
+/// Determines if the UI should be loaded synchronously. When false, loading occurs asynchronously.
+/// </summary>
-public bool LoadSynchronously;
+public bool LoadSynchronously = false;Line range hint 83-97: Consider optimizing the config validation loop.
While the cleanup logic is correct, it performs a linear search for each config. For large lists, this could be inefficient.
Consider pre-computing valid configs:
+ var validConfigs = new HashSet<string>(_configs.Select(c => c.UiType));
var cleanedConfigList = new List<string>(_sets[i].UiConfigsType.Count);
foreach (var uiConfig in _sets[i].UiConfigsType)
{
- if (_configs.FindIndex(config => config.UiType == uiConfig) > -1)
+ if (validConfigs.Contains(uiConfig))
{
cleanedConfigList.Add(uiConfig);
}
}Editor/UiConfigsEditor.cs (2)
113-136: Consider simplifying the FindIndex expression.
The implementation is solid, but the nested expression could be more readable.
Consider this simpler version:
- selectedIndex = uiPresentersAssemblyNames.FindIndex(uiPresenterName =>
- _scriptableObject.LoadingSpinnerTypeString ==
- uiPresenterName);
+ selectedIndex = uiPresentersAssemblyNames.FindIndex(name =>
+ _scriptableObject.LoadingSpinnerTypeString == name);Line range hint 225-242: Consider caching FindIndex results.
The FindIndex operation in the drawElementCallback could be expensive for large lists. Consider caching the results.
list.drawElementCallback = (rect, index, active, focused) =>
{
var serializedProperty = property.GetArrayElementAtIndex(index);
+ // Cache this dictionary at list creation time
+ var typeIndexCache = new Dictionary<string, int>();
var typeIndex = string.IsNullOrEmpty(serializedProperty.stringValue) ?
- 0 : _uiConfigsType.FindIndex(type => type == serializedProperty.stringValue);
+ 0 : typeIndexCache.GetValueOrDefault(serializedProperty.stringValue,
+ () => _uiConfigsType.FindIndex(type => type == serializedProperty.stringValue));
serializedProperty.stringValue = _uiConfigsType[EditorGUI.Popup(rect, typeIndex, _uiConfigsAddress)];
};Runtime/DelayUiPresenter.cs (2)
50-50: Correct typos in the comment for clarity.
In the comment, correct "inherithance" to "inheritance" and "it's" to "its" for proper grammar.
Apply this diff to fix the comment:
-// Not pretty inherithance with it's dependency but it works
+// The inheritance with its dependency is not ideal, but it works93-94: Clarify the XML documentation comment for OnEditorValidate().
The summary comment can be rephrased for better clarity regarding its purpose and when it is called.
Apply this diff to improve the comment:
-/// Called only in the Editor. Called in the end of this object MonoBehaviour's OnValidate() -> <see cref="OnValidate"/>
+/// Called only in the Editor at the end of this MonoBehaviour's `OnValidate()` method. See <see cref="OnValidate"/>.Runtime/IUiService.cs (7)
12-12: Correct grammatical error in documentation
The sentence in the summary should be rephrased for clarity and correctness.
Current:
The higher the layer the more close is to the camera viewport
Suggested change:
The higher the layer, the closer it is to the camera viewport
17-17: Fix capitalization in summary comment
In the summary, unnecessary capitalization is used.
Current:
Gets a read-only dictionary of the Presenters currently Loaded in memory by the UI service.
Suggested change:
Gets a read-only dictionary of the presenters currently loaded in memory by the UI service.
27-27: Improve clarity in documentation
The summary comment can be rephrased for better understanding.
Current:
Gets a read-only dictionary of the containers of UI, called 'Ui Set' maintained by the UI service.
Suggested change:
Gets a read-only dictionary of UI sets, which are containers of UI maintained by the UI service.
81-81: Consider returning IReadOnlyList instead of List
The method RemoveUiSet(int setId) returns a mutable List<UiPresenter>. To prevent unintended modifications to the returned collection, consider returning an IReadOnlyList<UiPresenter>.
148-148: Consider returning IReadOnlyList instead of List
The method GetAllVisibleUi() returns a mutable List<Type>. To ensure the list of visible UI types remains immutable to the caller, consider returning an IReadOnlyList<Type>.
167-173: Add XML documentation for the initialData parameter
The method OpenUiAsync<T, TData>(TData initialData) is missing documentation for the initialData parameter. Please add a <param> tag to describe this parameter.
181-181: Add XML documentation for the initialData parameter
The method OpenUiAsync<TData>(Type type, TData initialData) lacks documentation for the initialData parameter. Adding a <param> tag will improve code readability and maintain consistency.
Runtime/UiService.cs (1)
149-151: Avoid unnecessary warnings in LoadUiAsync
When a UI is already loaded, a warning is logged. This might not be necessary and could clutter the logs.
Consider checking if the UI is already loaded before calling LoadUiAsync, or adjust the log level to Debug if this is expected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
- CHANGELOG.md (1 hunks)
- Editor/NonDrawingViewEditor.cs (1 hunks)
- Editor/UiConfigsEditor.cs (8 hunks)
- Runtime/AnimatedUiPresenter.cs (0 hunks)
- Runtime/DelayUiPresenter.cs (1 hunks)
- Runtime/DelayUiPresenter.cs.meta (1 hunks)
- Runtime/Delayers.meta (1 hunks)
- Runtime/Delayers/AnimationDelayer.cs (1 hunks)
- Runtime/Delayers/PresenterDelayerBase.cs (1 hunks)
- Runtime/Delayers/PresenterDelayerBase.cs.meta (1 hunks)
- Runtime/Delayers/TimeDelayer.cs (1 hunks)
- Runtime/Delayers/TimeDelayer.cs.meta (1 hunks)
- Runtime/GameLovers.UiService.asmdef (1 hunks)
- Runtime/IUiService.cs (1 hunks)
- Runtime/LoadSynchronouslyAttribute.cs (1 hunks)
- Runtime/LoadSynchronouslyAttribute.cs.meta (1 hunks)
- Runtime/SafeAreaHelperView.cs (5 hunks)
- Runtime/UiAssetLoader.cs (3 hunks)
- Runtime/UiConfigs.cs (5 hunks)
- Runtime/UiPresenter.cs (2 hunks)
- Runtime/UiService.cs (3 hunks)
💤 Files with no reviewable changes (1)
- Runtime/AnimatedUiPresenter.cs
✅ Files skipped from review due to trivial changes (8)
- Editor/NonDrawingViewEditor.cs
- Runtime/DelayUiPresenter.cs.meta
- Runtime/Delayers.meta
- Runtime/Delayers/PresenterDelayerBase.cs.meta
- Runtime/Delayers/TimeDelayer.cs.meta
- Runtime/GameLovers.UiService.asmdef
- Runtime/LoadSynchronouslyAttribute.cs
- Runtime/LoadSynchronouslyAttribute.cs.meta
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~21-~21: The correct preposition here is “on” or “about”.
Context: ...Config* to know contain the information of the UiPresenter if is loaded sync or ...
(INFORMATION_OF)
🔇 Additional comments (21)
Runtime/Delayers/TimeDelayer.cs (1)
1-15: LGTM! Well-structured class implementation.
The class is well-documented and properly implements the serialized delay fields for Unity inspector integration. The inheritance from PresenterDelayerBase aligns with the PR objectives for introducing delayed UI actions.
Runtime/Delayers/AnimationDelayer.cs (1)
1-36: Verify integration with DelayUiPresenter.
The implementation looks solid but let's verify its integration with the new DelayUiPresenter system.
✅ Verification successful
Let me gather more context about the integration by checking for related classes and inheritance patterns.
Integration with DelayUiPresenter is properly implemented
The verification shows that:
DelayUiPresenterandDelayUiPresenterData<T>both have a serializedPresenterDelayerBasefield, allowing them to work withAnimationDelayerAnimationDelayercorrectly inherits fromPresenterDelayerBase- The change is documented in the changelog, indicating it's part of a planned feature set
- The implementation follows the expected inheritance pattern for delayed presenters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of AnimationDelayer with DelayUiPresenter
echo "Checking AnimationDelayer usage patterns..."
rg -l "DelayUiPresenter.*AnimationDelayer" || echo "No direct usage found"
# Look for migration examples or tests
echo -e "\nChecking for tests and examples..."
fd -e cs -e unity "Test.*Animation.*Delay|Example.*Animation.*Delay"
Length of output: 350
Script:
#!/bin/bash
# Check for DelayUiPresenter implementation
echo "Checking DelayUiPresenter implementation..."
rg -A 5 "class DelayUiPresenter"
# Check for other implementations of PresenterDelayerBase
echo -e "\nChecking other PresenterDelayerBase implementations..."
ast-grep --pattern 'class $_ : PresenterDelayerBase'
# Check for usage of AnimationDelayer in scenes or prefabs
echo -e "\nChecking for potential references in Unity scenes/prefabs..."
fd -e unity . | xargs rg "AnimationDelayer"
Length of output: 1398
Runtime/Delayers/PresenterDelayerBase.cs (1)
7-26: LGTM! Well-designed interface with clear contracts.
The interface is well-documented and provides a clean abstraction for presenter delay functionality.
Runtime/UiPresenter.cs (3)
29-35: LGTM! Clean implementation of self-closing capability.
The method provides a clean way for presenters to close themselves while maintaining proper service coordination.
65-82: Verify thread safety of gameObject access.
The null check is good, but consider potential race conditions in async contexts. Also, verify that UnloadUi properly cleans up all resources.
#!/bin/bash
# Check for async/await usage with InternalClose
ast-grep --pattern 'async $_ $_($$$) {
$$$
InternalClose($$$)
$$$
}'Consider adding debug logging for destroy operations to help diagnose UI lifecycle issues in production.
22-22: Verify the necessity of protected access.
Making _uiService protected exposes internal service functionality to derived classes. While this provides flexibility, it could lead to tight coupling and potential misuse.
✅ Verification successful
Protected access modifier is justified and well-encapsulated
The protected access to _uiService is appropriate because:
- It's used within the base
UiPresenterclass for core UI operations (Close,UnloadUi) - The service is properly initialized through an internal
Initmethod, maintaining controlled access - Only one derived class exists (
DelayUiPresenter), showing limited inheritance usage - No evidence of direct access to
_uiServicein derived classes was found
The design maintains encapsulation while providing necessary flexibility for the UI hierarchy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for direct access to _uiService in derived classes
ast-grep --pattern 'class $_ : UiPresenter {
$$$
$_() {
$$$
this._uiService
$$$
}
$$$
}'
Length of output: 483
Script:
#!/bin/bash
# Search for derived classes of UiPresenter
rg "class \w+ *: *UiPresenter" -l
# Search for any usage of _uiService in the codebase
rg "_uiService" -A 2 -B 2
# Look for any public methods in UiPresenter that might be using _uiService
ast-grep --pattern 'class UiPresenter {
$$$
public $_ $_($$$) {
$$$
_uiService
$$$
}
$$$
}'
Length of output: 1030
Runtime/UiConfigs.cs (2)
57-58: LGTM! Efficient implementation using ConvertAll.
The conversion logic is clean and type-safe through implicit operators.
Line range hint 150-156: LGTM! Good use of ReadOnly collection.
The change to return a read-only collection aligns with the PR objectives and prevents unintended modifications.
CHANGELOG.md (3)
7-8: LGTM! Version and date format follow semantic versioning.
The version number and date format comply with semantic versioning and Keep a Changelog conventions.
9-12: LGTM! Clear description of new features.
The additions are well documented with clear explanations of the new components and their purposes.
7-21: Consider adding deprecation notices for removed components.
The changelog indicates several removals (AnimatedUiPresenter, UiCloseActivePresenter, etc.) but doesn't specify if these were deprecated in previous versions. Consider adding information about:
- Whether these removals were previously announced
- The deprecation timeline
- Clear migration paths for each removed component
Let's check if these components were marked as deprecated in previous versions:
🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: The correct preposition here is “on” or “about”.
Context: ...Config* to know contain the information of the UiPresenter if is loaded sync or ...
(INFORMATION_OF)
Editor/UiConfigsEditor.cs (5)
58-67: LGTM! Clear and helpful tooltips.
The tooltips provide excellent guidance about UI configuration and layer ordering.
197-200: LGTM! Proper Unity serialization handling.
The implementation correctly handles Unity's serialization lifecycle with proper dirty marking and asset saving.
286-293: LGTM! Proper handling of addressable assets.
The implementation correctly handles readonly groups and follows AddressableAsset system best practices.
269-274: Verify impact of optional Canvas component.
The code now handles missing Canvas components gracefully, but we should ensure this doesn't affect existing UIs.
Let's check for existing Canvas dependencies:
#!/bin/bash
# Search for required Canvas references in UI presenters
ast-grep --pattern 'class $_ : UiPresenter {
$$$
Canvas $_
$$$
}'176-183: Consider adding error handling for asset loading.
While the LoadSynchronously attribute check is well implemented, consider adding error handling for cases where the asset loading fails.
Let's verify if there are any error handlers in the codebase:
Runtime/UiAssetLoader.cs (1)
59-59: Correct usage of Addressables.ReleaseInstance
The UnloadAsset method correctly uses Addressables.ReleaseInstance(asset) to release instantiated assets. This ensures that the asset is properly released from memory.
Runtime/DelayUiPresenter.cs (1)
74-75:
Initialize _delayer at runtime in DelayUiPresenterData<T> to prevent NullReferenceException.
Similar to DelayUiPresenter, the _delayer field in DelayUiPresenterData<T> is assigned in OnValidate(), which does not run at runtime. To avoid potential NullReferenceException errors, initialize _delayer in Awake() or OnEnable().
Apply this diff to ensure _delayer is assigned during runtime:
+private void Awake()
+{
+ _delayer = _delayer ?? GetComponent<PresenterDelayerBase>();
+}Likely invalid or redundant comment.
Runtime/SafeAreaHelperView.cs (1)
25-25: Addition of _initSizeDelta variable
The introduction of the private variable _initSizeDelta effectively stores the initial sizeDelta of the RectTransform, which is useful for recalculations.
Runtime/IUiService.cs (2)
58-59: Verify default interface method implementations
The interface IUiService contains default method implementations using expression-bodied members, such as:
bool RemoveUi<T>() where T : UiPresenter => RemoveUi(typeof(T));Ensure that the project's target framework and C# version support default interface methods (C# 8.0 or later).
225-227: Verify the usage of <inheritdoc /> in interface documentation
The <inheritdoc /> tag is used in the interface IUiServiceInit. Since interfaces do not inherit documentation like classes, ensure that this tag is necessary or remove it if it's not providing value.
| [SerializeField] protected Animation _animation; | ||
| [SerializeField] protected AnimationClip _introAnimationClip; | ||
| [SerializeField] protected AnimationClip _outroAnimationClip; |
There was a problem hiding this comment.
Add component and field validation.
The serialized fields lack validation, which could lead to NullReferenceException at runtime.
Consider adding validation:
+[RequireComponent(typeof(Animation))]
public class AnimationDelayer : PresenterDelayerBase
{
- [SerializeField] protected Animation _animation;
- [SerializeField] protected AnimationClip _introAnimationClip;
- [SerializeField] protected AnimationClip _outroAnimationClip;
+ [SerializeField, Required] protected Animation _animation;
+ [SerializeField, Required] protected AnimationClip _introAnimationClip;
+ [SerializeField, Required] protected AnimationClip _outroAnimationClip;
+
+ private void Awake()
+ {
+ if (_animation == null) _animation = GetComponent<Animation>();
+
+ if (_introAnimationClip == null)
+ Debug.LogError($"[{nameof(AnimationDelayer)}] Intro animation clip is required", this);
+ if (_outroAnimationClip == null)
+ Debug.LogError($"[{nameof(AnimationDelayer)}] Outro animation clip is required", this);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [SerializeField] protected Animation _animation; | |
| [SerializeField] protected AnimationClip _introAnimationClip; | |
| [SerializeField] protected AnimationClip _outroAnimationClip; | |
| [RequireComponent(typeof(Animation))] | |
| public class AnimationDelayer : PresenterDelayerBase | |
| { | |
| [SerializeField, Required] protected Animation _animation; | |
| [SerializeField, Required] protected AnimationClip _introAnimationClip; | |
| [SerializeField, Required] protected AnimationClip _outroAnimationClip; | |
| private void Awake() | |
| { | |
| if (_animation == null) _animation = GetComponent<Animation>(); | |
| if (_introAnimationClip == null) | |
| Debug.LogError($"[{nameof(AnimationDelayer)}] Intro animation clip is required", this); | |
| if (_outroAnimationClip == null) | |
| Debug.LogError($"[{nameof(AnimationDelayer)}] Outro animation clip is required", this); | |
| } |
| /// <inheritdoc /> | ||
| public override float OpenDelayInSeconds => _introAnimationClip.length * 1000; | ||
|
|
||
| /// <inheritdoc /> | ||
| public override float CloseDelayInSeconds => _outroAnimationClip.length * 1000; |
There was a problem hiding this comment.
Improve delay calculation clarity and safety.
The delay calculations need better documentation and null safety. The conversion from seconds to milliseconds (×1000) should be more explicit.
Consider these improvements:
+ /// <summary>
+ /// Gets the intro animation duration in milliseconds.
+ /// </summary>
public override float OpenDelayInSeconds =>
- _introAnimationClip.length * 1000;
+ _introAnimationClip != null ? _introAnimationClip.length * 1000 : 0f;
+ /// <summary>
+ /// Gets the outro animation duration in milliseconds.
+ /// </summary>
public override float CloseDelayInSeconds =>
- _outroAnimationClip.length * 1000;
+ _outroAnimationClip != null ? _outroAnimationClip.length * 1000 : 0f;
+ private const float MILLISECONDS_MULTIPLIER = 1000f;Committable suggestion was skipped due to low confidence.
| protected override void OnOpenStarted() | ||
| { | ||
| _animation.clip = _introAnimationClip; | ||
| _animation.Play(); | ||
| } | ||
|
|
||
| protected override void OnCloseStarted() | ||
| { | ||
| _animation.clip = _outroAnimationClip; | ||
| _animation.Play(); | ||
| } |
There was a problem hiding this comment.
Add error handling and state validation to animation methods.
The animation methods need proper error handling and state validation to ensure robust operation.
Consider these improvements:
protected override void OnOpenStarted()
{
+ if (!_animation || !_animation.enabled)
+ {
+ Debug.LogError($"[{nameof(AnimationDelayer)}] Animation component is missing or disabled", this);
+ return;
+ }
+
_animation.clip = _introAnimationClip;
- _animation.Play();
+ if (!_animation.Play())
+ {
+ Debug.LogWarning($"[{nameof(AnimationDelayer)}] Failed to play intro animation", this);
+ }
}
protected override void OnCloseStarted()
{
+ if (!_animation || !_animation.enabled)
+ {
+ Debug.LogError($"[{nameof(AnimationDelayer)}] Animation component is missing or disabled", this);
+ return;
+ }
+
_animation.clip = _outroAnimationClip;
- _animation.Play();
+ if (!_animation.Play())
+ {
+ Debug.LogWarning($"[{nameof(AnimationDelayer)}] Failed to play outro animation", this);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected override void OnOpenStarted() | |
| { | |
| _animation.clip = _introAnimationClip; | |
| _animation.Play(); | |
| } | |
| protected override void OnCloseStarted() | |
| { | |
| _animation.clip = _outroAnimationClip; | |
| _animation.Play(); | |
| } | |
| protected override void OnOpenStarted() | |
| { | |
| if (!_animation || !_animation.enabled) | |
| { | |
| Debug.LogError($"[{nameof(AnimationDelayer)}] Animation component is missing or disabled", this); | |
| return; | |
| } | |
| _animation.clip = _introAnimationClip; | |
| if (!_animation.Play()) | |
| { | |
| Debug.LogWarning($"[{nameof(AnimationDelayer)}] Failed to play intro animation", this); | |
| } | |
| } | |
| protected override void OnCloseStarted() | |
| { | |
| if (!_animation || !_animation.enabled) | |
| { | |
| Debug.LogError($"[{nameof(AnimationDelayer)}] Animation component is missing or disabled", this); | |
| return; | |
| } | |
| _animation.clip = _outroAnimationClip; | |
| if (!_animation.Play()) | |
| { | |
| Debug.LogWarning($"[{nameof(AnimationDelayer)}] Failed to play outro animation", this); | |
| } | |
| } |
| /// <param name="setId">The ID of the UI set to load from.</param> | ||
| /// <returns>An array of tasks that complete with each loaded UI.</returns> | ||
| /// <exception cref="KeyNotFoundException">Thrown if the service does not contain a UI set with the specified ID.</exception> | ||
| Task<Task<UiPresenter>>[] LoadUiSetAsync(int setId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the return type of LoadUiSetAsync
The method returns an array of Task<Task<UiPresenter>>, which can be confusing.
Current signature:
Task<Task<UiPresenter>>[] LoadUiSetAsync(int setId);Consider changing the return type to a single Task that returns an array of UiPresenter:
Task<UiPresenter[]> LoadUiSetAsync(int setId);This would simplify the asynchronous consumption of the method.
| for (int i = 0; i < _visibleUiList.Count; i++) | ||
| { | ||
| var reference = GetReference(_visibleUiList[i]); | ||
| if (reference.Layer == layer) | ||
| { | ||
| reference.Presenter.InternalClose(); | ||
| _visibleUiList.Remove(reference.UiType); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void AddUiSet(UiSetConfig uiSet) | ||
| { | ||
| if (_uiSets.ContainsKey(uiSet.SetId)) | ||
| { | ||
| throw new ArgumentException($"The Ui Configuration with the id {uiSet.SetId.ToString()} was already added"); | ||
| } | ||
|
|
||
| _uiSets.Add(uiSet.SetId, uiSet); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public List<UiPresenter> RemoveUiPresentersFromSet(int setId) | ||
| { | ||
| var set = GetUiSet(setId); | ||
| var list = new List<UiPresenter>(); | ||
| var type = _visibleUiList[i]; | ||
|
|
||
| for (int i = 0; i < set.UiConfigsType.Count; i++) | ||
| { | ||
| if (!HasUiPresenter(set.UiConfigsType[i])) | ||
| if (_uiConfigs[type].Layer == layer) | ||
| { | ||
| continue; | ||
| _uiPresenters[type].InternalClose(false); | ||
| _visibleUiList.Remove(type); |
There was a problem hiding this comment.
Fix collection modification while iterating in CloseAllUi(int layer)
Removing items from _visibleUiList while iterating can cause issues.
Iterate backwards or collect types to remove:
Option 1: Iterate backwards.
- for (int i = 0; i < _visibleUiList.Count; i++)
+ for (int i = _visibleUiList.Count - 1; i >= 0; i--)
{
var type = _visibleUiList[i];
if (_uiConfigs[type].Layer == layer)
{
_uiPresenters[type].InternalClose(false);
_visibleUiList.Remove(type);
}
}Option 2: Collect types to remove and remove them after iteration.
var typesToRemove = new List<Type>();
foreach (var type in _visibleUiList)
{
if (_uiConfigs[type].Layer == layer)
{
_uiPresenters[type].InternalClose(false);
typesToRemove.Add(type);
}
}
foreach (var type in typesToRemove)
{
_visibleUiList.Remove(type);
}| private async Task<UiPresenter> GetOrLoadUiAsync(Type type) | ||
| { | ||
| var set = GetUiSet(setId); | ||
|
|
||
| if (closeVisibleUi) | ||
| if (!_uiPresenters.TryGetValue(type, out var ui)) | ||
| { | ||
| var list = new List<Type>(set.UiConfigsType); | ||
| for (var i = 0; i < _visibleUiList.Count; i++) | ||
| { | ||
| if (list.Contains(_visibleUiList[i])) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| CloseUi(_visibleUiList[i]); | ||
| } | ||
| OpenLoadingSpinner(); | ||
| await LoadUiAsync(type); | ||
| CloseLoadingSpinner(); | ||
| } | ||
|
|
||
| for (var i = 0; i < set.UiConfigsType.Count; i++) | ||
| { | ||
| if (_visibleUiList.Contains(set.UiConfigsType[i])) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| OpenUi(set.UiConfigsType[i]); | ||
| } | ||
| return ui; |
There was a problem hiding this comment.
Assign ui after loading in GetOrLoadUiAsync
After calling LoadUiAsync, the ui variable is not updated. This means ui could be null when returned.
Assign ui after loading:
private async Task<UiPresenter> GetOrLoadUiAsync(Type type)
{
if (!_uiPresenters.TryGetValue(type, out var ui))
{
OpenLoadingSpinner();
await LoadUiAsync(type);
CloseLoadingSpinner();
+ ui = _uiPresenters[type];
}
return ui;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task<UiPresenter> GetOrLoadUiAsync(Type type) | |
| { | |
| var set = GetUiSet(setId); | |
| if (closeVisibleUi) | |
| if (!_uiPresenters.TryGetValue(type, out var ui)) | |
| { | |
| var list = new List<Type>(set.UiConfigsType); | |
| for (var i = 0; i < _visibleUiList.Count; i++) | |
| { | |
| if (list.Contains(_visibleUiList[i])) | |
| { | |
| continue; | |
| } | |
| CloseUi(_visibleUiList[i]); | |
| } | |
| OpenLoadingSpinner(); | |
| await LoadUiAsync(type); | |
| CloseLoadingSpinner(); | |
| } | |
| for (var i = 0; i < set.UiConfigsType.Count; i++) | |
| { | |
| if (_visibleUiList.Contains(set.UiConfigsType[i])) | |
| { | |
| continue; | |
| } | |
| OpenUi(set.UiConfigsType[i]); | |
| } | |
| return ui; | |
| private async Task<UiPresenter> GetOrLoadUiAsync(Type type) | |
| { | |
| if (!_uiPresenters.TryGetValue(type, out var ui)) | |
| { | |
| OpenLoadingSpinner(); | |
| await LoadUiAsync(type); | |
| CloseLoadingSpinner(); | |
| ui = _uiPresenters[type]; | |
| } | |
| return ui; |
| public IReadOnlyDictionary<Type, UiPresenter> LoadedPresenters => new Dictionary<Type, UiPresenter>(_uiPresenters); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyDictionary<int, GameObject> Layers => new Dictionary<int, GameObject>(_layers); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyDictionary<int, UiSetConfig> UiSets => new Dictionary<int, UiSetConfig>(_uiSets); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ReadOnlyDictionary to prevent unnecessary allocations
The properties LoadedPresenters, Layers, and UiSets create new dictionary instances on each access, which can affect performance.
Consider using ReadOnlyDictionary to wrap the existing dictionaries without copying them:
- public IReadOnlyDictionary<Type, UiPresenter> LoadedPresenters => new Dictionary<Type, UiPresenter>(_uiPresenters);
+ public IReadOnlyDictionary<Type, UiPresenter> LoadedPresenters => new ReadOnlyDictionary<Type, UiPresenter>(_uiPresenters);
- public IReadOnlyDictionary<int, GameObject> Layers => new Dictionary<int, GameObject>(_layers);
+ public IReadOnlyDictionary<int, GameObject> Layers => new ReadOnlyDictionary<int, GameObject>(_layers);
- public IReadOnlyDictionary<int, UiSetConfig> UiSets => new Dictionary<int, UiSetConfig>(_uiSets);
+ public IReadOnlyDictionary<int, UiSetConfig> UiSets => new ReadOnlyDictionary<int, UiSetConfig>(_uiSets);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public IReadOnlyDictionary<Type, UiPresenter> LoadedPresenters => new Dictionary<Type, UiPresenter>(_uiPresenters); | |
| /// <inheritdoc /> | |
| public IReadOnlyDictionary<int, GameObject> Layers => new Dictionary<int, GameObject>(_layers); | |
| /// <inheritdoc /> | |
| public IReadOnlyDictionary<int, UiSetConfig> UiSets => new Dictionary<int, UiSetConfig>(_uiSets); | |
| public IReadOnlyDictionary<Type, UiPresenter> LoadedPresenters => new ReadOnlyDictionary<Type, UiPresenter>(_uiPresenters); | |
| /// <inheritdoc /> | |
| public IReadOnlyDictionary<int, GameObject> Layers => new ReadOnlyDictionary<int, GameObject>(_layers); | |
| /// <inheritdoc /> | |
| public IReadOnlyDictionary<int, UiSetConfig> UiSets => new ReadOnlyDictionary<int, UiSetConfig>(_uiSets); |
*Changed:
Summary by CodeRabbit
Release Notes for Version 0.8.0
New Features
PresenterDelayerBase,AnimationDelayer,TimeDelayer, andDelayUiPresenter.SafeAreaHelperViewfor better position and size adjustments based on safe areas.LoadSynchronouslyAttributeto mark presenters for synchronous loading.Bug Fixes
UiServicefor duplicate configurations and presenters.Documentation
Refactor
IUiServiceinterface andUiAssetLoaderfor better usability and performance.Chores
package.jsonfor versioning and dependencies.