fix(AccountInfoDisplay): culture-safe PnL coloring + render cleanup#169
Open
AlbertoAmadorBelchistim wants to merge 4 commits into
Open
Conversation
Replace format-then-parse roundtrip with a record-backed line list that carries the raw decimal alongside the formatted string. The previous ExtractNumericValue stripped commas and re-parsed the N2-formatted value with the current culture, which fails on cultures where ',' is the decimal separator (es-ES, fr-FR, de-DE, ...) for any PnL >= 1000 absolute. Parse failure yielded 0, collapsing positive and negative PnL into the neutral colour. Coloring decisions now read the original Portfolio.OpenPnL / Portfolio.ClosedPnL values directly. ExtractNumericValue and BuildDisplayText (string-with-pipe-delimiter contract) are removed.
The border RenderPen was instantiated on every OnRender call, and the RenderFont was reassigned on every FontSize change without disposing the previous instance. Both wrap GDI+ resources; reuse them and dispose explicitly when replaced.
_stringFormat was constructed but never passed to any DrawString call since the indicator draws by absolute coordinates. Remove the field. Promote the rendering padding to a private const. The Container?.Region null check effectively only validates Container (Region is a struct); make that explicit so the intent reads clearly.
Collaborator
|
Reviewed and tested locally against current Develop. The functional change looks good to me: PnL coloring now uses the raw decimal values instead of parsing formatted strings, so the culture-dependent coloring issue should be fixed. I also verified that the PR builds successfully, and all four pending PRs merge together into Develop without conflicts or build errors. Minor non-blocking cleanup:
No blocking issues from my side. OK to merge after optional cleanup. |
- Use cached _borderPen instead of allocating per render - Remove unused `var old = _font` from FontSize setter - Fix mojibake in Total PnL comment (em-dash -> ASCII)
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
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.
Summary
Three small, independent changes to AccountInfoDisplay's render path:
Bug fix - PnL coloring failed on cultures where
,is the decimal separator (es-ES, fr-FR, de-DE, ...) for any PnL ≥ 1000 absolute.The old
ExtractNumericValueformatted withN2then re-parsed after stripping commas, which produced an invalid thousand-grouping in those cultures and the parse silently returned 0 — collapsing positive and negative PnL into the neutral colour.Perf - the border
RenderPenwas instantiated on everyOnRendercall. Cached as a field.Cleanup
_stringFormatwas constructed but never passed to anyDrawString. Removed.private const.Container?.Region == nullguard made explicit (Regionis a struct, so the null-conditional only checksContainer).No public API changes.
Commits
fix(account-info): culture-safe value coloringperf(account-info): cache border RenderPen across renderschore(account-info): drop dead RenderStringFormat field; tighten guardNotes
Total PnLcontinues to useOpenPnL + ClosedPnL(session-level) rather thanPortfolio.TotalPnL(cumulative). This is intentional and now documented inline inBuildLines.RenderFontandRenderPenare sealed and do not implementIDisposablein the SDK, so no explicit disposal is required when references are replaced.Verification