Conversation
+ Switch to source-generated Locale with binding support + Move Locale class from Hi3Helper.Core to CollapseLauncher project scope
neon-nyan
added
Enhancement
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return 0; | ||
| } | ||
|
|
||
| // Get first value reference | ||
| TKey? firstKey = dict.Keys.FirstOrDefault(); | ||
| if (firstKey == null) | ||
| { | ||
| throw new InvalidOperationException(); | ||
| } | ||
| ref TValue firstRef = ref CollectionsMarshal.GetValueRefOrNullRef(dict, firstKey); | ||
| if (Unsafe.IsNullRef(ref firstRef)) |
There was a problem hiding this comment.
Bug: The GetValueIndexFromKey method uses unsafe pointer arithmetic to find a dictionary value's index, which is unreliable and not guaranteed to work by the .NET runtime.
Severity: CRITICAL
Suggested Fix
Replace the unsafe pointer arithmetic in GetValueIndexFromKey. A reliable approach would be to convert the dictionary's keys or values to a list and use the IndexOf() method to find the correct index. For example: dict.Keys.ToList().IndexOf(key).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
CollapseLauncher/Classes/Helper/Metadata/LauncherMetadataHelper.GameTitleAndRegion.cs#L83-L100
Potential issue: The `GetValueIndexFromKey` method calculates a dictionary value's index
using unsafe pointer arithmetic. It computes the byte offset between the first value's
reference and the target value's reference, then divides by a hardcoded or calculated
size. This approach incorrectly assumes that a .NET `Dictionary<TKey, TValue>` stores
its values in a contiguous memory block ordered by insertion, which is not a guaranteed
contract. This faulty index is then used to select an item in a UI combo box, leading to
the wrong game region being displayed and potentially used, and also causes unnecessary
re-initialization of Discord's Rich Presence feature due to incorrect equality checks.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| // Initialize and clear the stamp dictionary | ||
| LauncherMetadataStampDictionary ??= new Dictionary<string, Stamp>(); | ||
| LauncherMetadataStampDictionary.Clear(); |
There was a problem hiding this comment.
Bug: The static list LauncherMetadataStamp is not cleared on re-initialization, leading to duplicate entries and a crash from an ArgumentException when populating a dictionary.
Severity: HIGH
Suggested Fix
Add LauncherMetadataStamp.Clear() at the beginning of the InitializeStamp method, before LauncherMetadataStampDictionary.Clear(), to ensure the list is empty before new stamp data is added. This will prevent the accumulation of duplicate entries across multiple initializations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: CollapseLauncher/Classes/Helper/Metadata/LauncherMetadataHelper.cs#L242
Potential issue: The `InitializeStamp` method is called during application
initialization. It clears `LauncherMetadataStampDictionary` but fails to clear the
underlying `LauncherMetadataStamp` list before adding new items with `AddRange`. If
`Initialize` is called more than once during the application's lifecycle (e.g., during
OOBE setup or on config re-initialization), the `LauncherMetadataStamp` list accumulates
duplicate entries. The subsequent logic attempts to add these items to the dictionary
using `dict.Add()`, which will throw an `ArgumentException` upon encountering a
duplicate key, causing the application to crash.
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
Main Goal
The localization has been rewritten entirely to enable couple of improvements for easier UI element maintainability and binding process. List of improvements in this PR as follows:
[New] Switching to source-generated JSON -> Class mapper & deserializer.
Previously, developer should map the property inside of the source-code to represents entries inside of the locale
.jsonfile. Thanks to source-generated mapper, the property will be populated and generated automatically each time new entries have been added to the.jsonfile.[New] Add binding support to the Locale object.
This enables each UI Elements that binds their represented Locale property, to be able to have its text updated while a locale is being changed to another language without the needs of manual binding update.
PR Status :
Templates
Changelog Prefixes