Fix (InitialBalance): lifecycle, validation and code-health cleanups#167
Open
AlbertoAmadorBelchistim wants to merge 7 commits into
Conversation
The constructor wires 9 ValueDataSeries.PropertyChanged handlers to DataSeriesPropertyChanged but never detaches them. Each time the indicator is removed and re-added on the same chart, the previous handlers stay alive on the orphan instances and accumulate across reloads. Add OnDispose that mirrors every += with a matching -= (including _mid, see follow-up commit) so the subscriptions are released when the indicator is disposed.
GetLabelTagSuffixes already returns "Mid" as the tag suffix for the Mid series, and UpdateLabelColors knows how to repaint the matching labels. But _mid was the only ValueDataSeries not subscribed to DataSeriesPropertyChanged in the constructor, so changing its color in the property grid did not refresh the "Mid" AddText labels until an unrelated repaint happened. Add the missing subscription so the Mid color now propagates to its labels consistently with the other 9 series.
Both StartDate and EndDate were declared with Order = 20 inside the SessionTime property group, which leaves their relative position in the property grid undefined. Bump EndDate to Order = 25 so StartTime is rendered first and EndTime second, matching the natural reading order.
The X1, X2 and X3 multiplier properties accepted any decimal value. Negative values invert the extension levels (IBHX* fall below the Initial Balance high and IBLX* rise above the Initial Balance low), producing crossed and visually nonsensical bands. Very large values push levels off the chart with no warning. Add [Range(0.1, 100)] to all three multipliers to keep the resulting extension levels above the Initial Balance high (for IBHX*) and below the Initial Balance low (for IBLX*).
The Pen alias declared at the top of the file is never referenced — every call site uses CrossPen directly. Drop the dead using directive.
The eight RangeDataSeries fields (_ibhx32, _ibhx21, _ibhx1h, _ibHm, _ibMl, _ibl1, _iblx12, _iblx23) are initialized at declaration and never reassigned. Marking them readonly matches the convention already used for the ten ValueDataSeries in the same file.
In Bars mode the session-end check is `bar - _lastStartBar >= Period` and _endTime is never read. The assignment `_endTime = candleFullDateTime.AddMinutes(_period)` runs anyway on every session start, with no observable effect. Wrap the assignment in `if (PeriodMode == PeriodType.Minutes)` to make the dependency between _endTime and PeriodMode explicit.
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.
InitialBalance: lifecycle, validation and code-health cleanups
This PR bundles a set of small, self-contained fixes and clean-ups for
InitialBalance.cs. Each commit is independent and easy to cherry-pick or revert; happy to split into separate PRs if preferred.Changes
Release
PropertyChangedsubscriptions inOnDispose— the constructor wires nineValueDataSeries.PropertyChangedhandlers and never detaches them, so they accumulate as zombie subscriptions across reloads. Adds the missingOnDisposethat mirrors every+=with a matching-=.Subscribe
_midtoPropertyChangedso its labels react to color changes —_midis the only series with a dedicated branch inGetLabelTagSuffixesthat was not subscribed in the constructor. As a result, changing the Mid color in the property grid did not refresh the"Mid"AddTextlabels. Adds the missing subscription for parity with the other nine series.Disambiguate
StartTime/EndTimedisplay order — both properties were declared withOrder = 20inside theSessionTimegroup, leaving their relative order in the property grid undefined. BumpsEndTimetoOrder = 25.Constrain
X1/X2/X3multipliers to a positive range — adds[Range(0.1, 100)]to the three multipliers. Negative values used to invert the extension levels (IBHX* below the IB high, IBLX* above the IB low).Remove the unused
using Pen = CrossPen;alias — the alias is never referenced; every site usesCrossPendirectly.Mark
RangeDataSeriesfields asreadonly— the eight fields are initialized at declaration and never reassigned. Matches the convention already used by the tenValueDataSeriesin the same file.Skip the
_endTimeassignment whenPeriodModeisBars— inBarsmode the session-end check isbar - _lastStartBar >= Periodand_endTimeis never read. Wraps the assignment inif (PeriodMode is PeriodType.Minutes)so the dependency between_endTimeandPeriodModeis explicit.End-user impact
SessionTimeproperties have a deterministic order (fix 3); multipliers cannot produce crossed bands (fix 4).Open questions for the maintainers
A few decisions I would rather defer to you than take unilaterally:
1. What is the intended state of the
_midseries?_midis initialized withCrossColor.FromArgb(0, 0, 255, 0)—A = 0, fully transparent — so the line and its"Mid"AddTextlabels are invisible by default. At the same time, the series is still calculated every bar, exposed in the property grid (color editable), described viaSessionAveragePriceDescriptionand labelled in theOnCalculatetext block. Two equally plausible interpretations:FromArgb(255, 0, 255, 0)). Follow-up: pick a sensible default such asDefaultColors.Green.Convert()."MidId"still load. Follow-up: replace the alpha=0 trick with the canonical hidden-series pattern (VisualType = VisualMode.Hide,IsHidden = true) and drop the now-dead"Mid"AddTextcall together with the_midbranch inGetLabelTagSuffixes.Could you confirm which one is intended? I held this fix out of the present PR on purpose and am happy to send the matching patch as a small follow-up.
2. Renaming the unprefixed value fields
The internal value variables
ibhx1..3,iblx1..3,mid(lines 199–206) are the only fields in the file without the_prefix that the rest of the private state uses. Renaming them to e.g._ibhx1Value..3,_iblx1Value..3,_midValuewould make the distinction between the value (ibhx1) and the correspondingValueDataSeries(_ibhx1) explicit and remove an easy-to-confuse shadow.The change would touch every call site inside
OnCalculateand the labels block — a noisy diff for a cosmetic gain — so I left it out of this PR. Would you take it as a separatechore: rename internal value fields in InitialBalancePR, or would you rather leave the current naming?3. Splitting drawing concerns out of
OnCalculatein a follow-up PRToday
OnCalculatemixes pure calculation (DataSeries writes) with drawing-side state management (tenAddTextcalls per bar,DrawingRectanglecreation and per-bar mutation inRectangles). The usual SDK pattern keepsOnCalculatefor state writes andOnRenderfor read-only drawing.Would you be open, in a future PR, to exploring whether the label management and the IB rectangle update could move to
OnRender? I would rather check the direction is acceptable before investing in the refactor.Thanks for reviewing.