Add Platinum & Ducat counting to 'Auto Add' window#411
Open
occluder wants to merge 6 commits intoWFCD:masterfrom
Open
Add Platinum & Ducat counting to 'Auto Add' window#411occluder wants to merge 6 commits intoWFCD:masterfrom
occluder wants to merge 6 commits intoWFCD:masterfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis change introduces aggregate totals tracking for Platinum and Ducats in the AutoAdd system. The ViewModel now maintains and updates these totals dynamically when items are added, removed, or when their options change. The UI is updated to display these calculated totals alongside associated icons. Changes
Sequence DiagramsequenceDiagram
participant UI as AutoCount UI
participant Item as AutoAddSingleItem
participant VM as AutoAddViewModel
UI->>Item: Change option selection
Item->>Item: Detect option change<br/>capture previousOption
Item->>VM: UpdateTotalsForOptionChange(previousOption, newOption)
VM->>VM: Read previous option data<br/>(if exists)
VM->>VM: Subtract from TotalPlatinum<br/>& TotalDucats
VM->>VM: Read new option data
VM->>VM: Add to TotalPlatinum<br/>& TotalDucats
VM->>VM: Raise property changed<br/>notifications
VM->>UI: Display updated TotalPlatinum<br/>& TotalDucats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Adding
|
Author
|
Thank you for the remedy to my headache, I was jumping around in code trying to debug it 😅 I believe the functionality to be complete now, so I will open the PR for review. Code has to be looked at, as I do not fully understand what the considerations ask of me |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
WFInfo/AutoAddViewModel.cs (2)
100-100: Public mutable field_parentwith underscore prefix — consider encapsulating.By convention, underscore-prefixed members are private backing fields. Exposing
_parentas a public field allows any caller to mutate it. Consider making it a read-only property set via the constructor, or at leastinternal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WFInfo/AutoAddViewModel.cs` at line 100, The public mutable field _parent on AutoAddViewModel breaks encapsulation and uses an underscore prefix reserved for private backing fields; change _parent to a private readonly field or expose it as a read-only property (e.g., Parent) set via the AutoAddViewModel constructor (or at minimum mark it internal instead of public) so external callers cannot mutate it directly and the naming follows conventions; update any callers to use the new constructor/property.
134-149: Constructor relies on_parentbeing null whenActiveOptionis first set — fragile ordering.
ActiveOptionis assigned at line 141 before_parentis set at line 147. This works today because the null-conditional_parent?.UpdateTotalsForOptionChange(...)is a no-op, andaddItemseparately accounts for the initial totals. However, if anyone reorders these two assignments (e.g. setting_parentfirst), the totals will be double-counted.Consider moving
_parent = parent;before theActiveOptionassignment and removing the redundantUpdateTotalsForOptionChangecall inaddItem, or add a brief comment explaining the required ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WFInfo/AutoAddViewModel.cs` around lines 134 - 149, Constructor AutoAddSingleItem sets ActiveOption before assigning _parent which makes behavior ordering-dependent; move the assignment _parent = parent to before setting ActiveOption inside AutoAddSingleItem (or alternatively remove the redundant call to UpdateTotalsForOptionChange inside addItem), so that ActiveOption's setter can safely call _parent?.UpdateTotalsForOptionChange without relying on null ordering; update or add a short comment on the constructor explaining the intended ordering if you choose to keep the current sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Around line 64-95: UpdateTotalsForOptionChange currently casts the result of
Main.dataBase.marketData.GetValue(previousOption/newOption) to JObject and then
indexes into it, which throws a NullReferenceException when GetValue returns
null; update the method to null-guard each lookup (check the result of GetValue
and ensure previousJob/newJob is not null before accessing ["plat"] or
["ducats"]), or better extract a small helper (e.g. TryGetMarketValues(string
key, out double plat, out int ducats) used by UpdateTotalsForOptionChange) to
centralize the GetValue, null-check, parsing (double.TryParse/int.TryParse) and
return success/failure so you avoid duplicated code and prevent NREs when a key
is missing.
---
Nitpick comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Line 100: The public mutable field _parent on AutoAddViewModel breaks
encapsulation and uses an underscore prefix reserved for private backing fields;
change _parent to a private readonly field or expose it as a read-only property
(e.g., Parent) set via the AutoAddViewModel constructor (or at minimum mark it
internal instead of public) so external callers cannot mutate it directly and
the naming follows conventions; update any callers to use the new
constructor/property.
- Around line 134-149: Constructor AutoAddSingleItem sets ActiveOption before
assigning _parent which makes behavior ordering-dependent; move the assignment
_parent = parent to before setting ActiveOption inside AutoAddSingleItem (or
alternatively remove the redundant call to UpdateTotalsForOptionChange inside
addItem), so that ActiveOption's setter can safely call
_parent?.UpdateTotalsForOptionChange without relying on null ordering; update or
add a short comment on the constructor explaining the intended ordering if you
choose to keep the current sequence.
Contributor
|
I suggest letting each |
Author
|
I have deliberately not implemented CodeRabbit's suggestions above as they require a higher language version. The code to get an item's value is copy pasted from another part of the codebase. Let me know if any changes are necessary |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
WFInfo/AutoAddViewModel.cs (1)
215-222:⚠️ Potential issue | 🟡 Minor
RaisePropertyChanged()inRemoveFromParentfires for a non-existent property.With no explicit argument,
[CallerMemberName]injects"RemoveFromParent"as the property name. No property by that name exists, so all listeners receive a spurious change event. If the intent is to signal that the item was removed, the notification is better placed on an actual property; if there is no such intent, the call should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WFInfo/AutoAddViewModel.cs` around lines 215 - 222, The RaisePropertyChanged() call in RemoveFromParent fires a spurious notification because CallerMemberName will emit "RemoveFromParent" which is not a real property; fix by either removing the RaisePropertyChanged() call entirely from RemoveFromParent if no property change should be announced, or replace it with a call that supplies the actual property name that changed (e.g., RaisePropertyChanged("Parent") or the relevant property like "IsRemoved" or the collection property that observers expect) and ensure that property (or an appropriate boolean flag) exists on the class; locate the RemoveFromParent method and the _parent.removeItem(this) call to apply the change.
🧹 Nitpick comments (2)
WFInfo/AutoAddViewModel.cs (2)
58-63: RedundantRecalculateTotals()call inremoveItem.
_itemList.Remove(item)already firesCollectionChanged, which callsRecalculateTotals(). The explicit call on line 62 is redundant.♻️ Proposed fix
public void removeItem(AutoAddSingleItem item) { item.PropertyChanged -= ItemChanged; _itemList.Remove(item); - RecalculateTotals(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WFInfo/AutoAddViewModel.cs` around lines 58 - 63, In removeItem(AutoAddSingleItem item) the explicit call to RecalculateTotals() is redundant because _itemList.Remove(item) triggers the CollectionChanged handler that already calls RecalculateTotals; remove the direct RecalculateTotals() invocation (keep the PropertyChanged unsubscribe and the _itemList.Remove(item) call intact) so totals are only recalculated via the collection change event.
27-44: Consider usingSetFieldto avoid spurious property-change notifications.Both
TotalPlatinumandTotalDucatsunconditionally callRaisePropertyChanged()even when the computed total is identical to the previous value. SinceRecalculateTotalsfires on everyItemChangedand everyCollectionChanged, this can produce unnecessary UI re-renders. TheSetFieldhelper on theINPCbase class already provides equality-gated notification.♻️ Proposed refactor
- public double TotalPlatinum - { - get => _totalPlatinum; - private set - { - _totalPlatinum = value; - RaisePropertyChanged(); - } - } - public int TotalDucats - { - get => _totalDucats; - private set - { - _totalDucats = value; - RaisePropertyChanged(); - } - } + public double TotalPlatinum + { + get => _totalPlatinum; + private set => SetField(ref _totalPlatinum, value); + } + public int TotalDucats + { + get => _totalDucats; + private set => SetField(ref _totalDucats, value); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@WFInfo/AutoAddViewModel.cs` around lines 27 - 44, The property setters for TotalPlatinum and TotalDucats currently assign and call RaisePropertyChanged unconditionally, causing spurious notifications when RecalculateTotals runs on ItemChanged/CollectionChanged; change both setters to use the INPC base helper SetField (or its equivalent) to perform equality-checked assignment and only raise PropertyChanged when the value actually changes so UI rerenders are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Around line 84-87: Remove the finalizer ~AutoAddViewModel() that unsubscribes
_itemList.CollectionChanged because detaching managed events in a finalizer is
unsafe and unnecessary; either delete the finalizer entirely from the
AutoAddViewModel class, or if explicit teardown is required implement
IDisposable on AutoAddViewModel and move the _itemList.CollectionChanged -=
CollectionChanged logic into Dispose() (ensuring Dispose is idempotent and
called by consumers).
- Around line 148-150: The ternary in ActiveOption uses a dead null check on
options (new ObservableCollection<string>(options) already throws if options is
null); remove the redundant "options != null" and read the selected value from
the newly assigned RewardOptions collection instead. Concretely, keep the
activeIndex bounds logic, and set ActiveOption = activeIndex >= 0 ?
RewardOptions[activeIndex] : "" (referencing RewardOptions, ActiveOption,
activeIndex and the ObservableCollection<string>(options) construction).
---
Outside diff comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Around line 215-222: The RaisePropertyChanged() call in RemoveFromParent fires
a spurious notification because CallerMemberName will emit "RemoveFromParent"
which is not a real property; fix by either removing the RaisePropertyChanged()
call entirely from RemoveFromParent if no property change should be announced,
or replace it with a call that supplies the actual property name that changed
(e.g., RaisePropertyChanged("Parent") or the relevant property like "IsRemoved"
or the collection property that observers expect) and ensure that property (or
an appropriate boolean flag) exists on the class; locate the RemoveFromParent
method and the _parent.removeItem(this) call to apply the change.
---
Duplicate comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Around line 156-171: UpdateValues currently casts the result of
Main.dataBase.marketData.GetValue(ActiveOption) to JObject and then dereferences
job["plat"] which throws NullReferenceException when the key is missing; modify
UpdateValues to capture the result into a JToken/JObject variable from
Main.dataBase.marketData.GetValue(ActiveOption), check for null (or use
TryGetValue if available) before accessing job["plat"] and job["ducats"], and if
missing set PlatinumValue and DucatValue to 0 (or the existing else-path) so
missing marketData entries (e.g. "Forma Blueprint") no longer crash the UI.
---
Nitpick comments:
In `@WFInfo/AutoAddViewModel.cs`:
- Around line 58-63: In removeItem(AutoAddSingleItem item) the explicit call to
RecalculateTotals() is redundant because _itemList.Remove(item) triggers the
CollectionChanged handler that already calls RecalculateTotals; remove the
direct RecalculateTotals() invocation (keep the PropertyChanged unsubscribe and
the _itemList.Remove(item) call intact) so totals are only recalculated via the
collection change event.
- Around line 27-44: The property setters for TotalPlatinum and TotalDucats
currently assign and call RaisePropertyChanged unconditionally, causing spurious
notifications when RecalculateTotals runs on ItemChanged/CollectionChanged;
change both setters to use the INPC base helper SetField (or its equivalent) to
perform equality-checked assignment and only raise PropertyChanged when the
value actually changes so UI rerenders are avoided.
Author
|
@D1firehail Any chance this could be looked at again? I'd like to have things ironed out before someone with merge capability takes a look |
Contributor
|
Yeah no problem. Not sure I'll have time today, but should be able to within a few days at most. |
D1firehail
reviewed
Mar 3, 2026
| { | ||
| public AutoAddViewModel _parent; | ||
|
|
||
| private const NumberStyles style = NumberStyles.AllowDecimalPoint | NumberStyles.AllowThousands | NumberStyles.AllowExponent; |
Contributor
There was a problem hiding this comment.
The necessity of this confused me a bit, but apparently the overload of double.TryParse that accepts a CultureInfo without NumberStyles doesn't exist in our .NET version.
Contributor
|
LGTM I could imagine similar info (and ducat efficiency) on a per-item level to be somewhat useful, but it's far from necessary and would require UI space considerations. No need to bother until/unless someone actually requests it. |
Author
|
Initially I had thoughts about having a plat/ducats per minute counter, but I have lost interest in that thought. If someone is interested in any of the mentioned ideas I would be happy to put in the effort |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Display cumulative Platinum and Ducats earned in 'Auto Add' view by; adding TotalPlatinum and TotalDucats to AutoAddViewModel.
Totals update automatically as items are added/removed.
I am however running into an issue with updating values when a relic reward option is changed (see line 118 here). So far, have not had any success investigating what is causing this, so this PR will be a draft until the issue and considerations below are addressed. Any help with this would be appreciated.Considerations
Summary by CodeRabbit