feat(series): add connectGaps option to line, area and baseline series#2075
feat(series): add connectGaps option to line, area and baseline series#2075leo01102 wants to merge 1 commit into
Conversation
fixes tradingview#699 This change introduces a new option connectGaps for continuous series types (Line, Area, Baseline). When set to alse, the renderer will break the line/area at gaps in the data (detected by non-contiguous time indices). It defaults to rue to maintain backward compatibility.
|
CI checks that fail are expected consequences of this PR, not regressions.
|
SlicedSilver
left a comment
There was a problem hiding this comment.
Thanks for the PR @leo01102
I've left some suggestions which should hopefully be clear enough.
One key behaviour difference I'm recommending is within the gap detection logic so that it should only consider gaps when a series has explicit whitespace added (instead of just a jump in the time index). If you would also like the gap detection to cover those other cases (gaps created due to other series with different time points) then I think we should have an option to enable that so that the developer has more control over the feature. Requiring explicit whitespace to be added to the series is the most robust and flexible approach for now; however it may not be the most convenient depending on your use-case.
| * | ||
| * @defaultValue `true` | ||
| */ | ||
| connectGaps?: boolean; |
There was a problem hiding this comment.
Suggestion: connectGaps should be required (not optional).
Every other property in this interface (e.g. pointMarkersVisible, lastPriceAnimation) is declared as required, and this property always has a default value provided in lineStyleDefaults. Declaring it as optional forces every consumer to cast with as boolean — there are currently 6 instances of options.connectGaps as boolean across the pane views.
| connectGaps?: boolean; | |
| connectGaps: boolean; |
The JSDoc @defaultValue is also misleading alongside ?: — it implies the value is always present (via defaults), but the type says it could be undefined.
Same change needed in AreaStyleOptions (line 367) and BaselineStyleOptions (line 527).
Once this is done, all six as boolean casts in line-pane-view.ts, area-pane-view.ts, and baseline-pane-view.ts should be removed.
| /** | ||
| * Connect gaps in data. | ||
| * | ||
| * @defaultValue `true` | ||
| */ |
There was a problem hiding this comment.
The documentation should explain more precisely what constitutes a "gap". Users may be confused about the interaction with multiple series on the same chart. I'd suggest something like:
| /** | |
| * Connect gaps in data. | |
| * | |
| * @defaultValue `true` | |
| */ | |
| /** | |
| * Whether to draw a connecting line across gaps (whitespace) in the data. | |
| * | |
| * When `true`, the line will be drawn continuously even if the data contains | |
| * whitespace entries. When `false`, the line will break at each whitespace gap. | |
| * | |
| * Note: Gaps are only detected where this series has explicit whitespace data | |
| * (entries with only a `time` property and no value). If another series on the | |
| * same chart has data at different time points, this will not create gaps in | |
| * this series; add whitespace entries to this series' data to create gaps. | |
| * | |
| * @defaultValue `true` | |
| */ |
Same JSDoc update needed for the identical blocks in AreaStyleOptions and BaselineStyleOptions.
| const itemStyle = styleGetter(renderingScope, currentItem); | ||
|
|
||
| const prevItem = items[i - 1]; | ||
| const isGap = !connectGaps && currentItem.time !== undefined && prevItem.time !== undefined && currentItem.time - prevItem.time > 1; |
There was a problem hiding this comment.
Two issues with this gap detection:
undefinedchecks are unnecessary. As noted in the comment on the generic constraint,timeis always present on these items. ThecurrentItem.time !== undefined && prevItem.time !== undefinedguards are dead code.time - prevItem.time > 1is too broad — it catches gaps from other series, not just whitespaceTimePointIndexis a global index shared across all series on a chart. When Series A has data at times[Jan 1, Jan 3]and Series B has data at[Jan 1, Jan 2, Jan 3], the merged time scale gives indices[0, 1, 2]. Series A's items will be[{time: 0}, {time: 2}]— a difference of 2, which this check treats as a gap. But Series A never provided whitespace; the index gap comes from Series B contributing a time point that Series A doesn't have.
The intended behaviour (per discussion) is: only break the line where this series has explicit whitespace data between two points. If the user wants gaps for timestamps that only exist in another series, they should add whitespace entries to this series.
This means the gap detection can't be done purely at the renderer level by comparing TimePointIndex values. The information about whether whitespace existed between two points is lost by the time we reach walkLine — whitespace rows are filtered out in DataLayer._setRowsToSeries().
One approach: during item construction in the pane view (e.g. in LinePaneViewBase), compute a boolean flag like gapBefore on each item by checking whether the series' original data contained whitespace rows between consecutive plot rows. This information is available at that layer and could be passed through to the renderer without adding complexity to walkLine's parameters.
| if (isGap) { | ||
| finishStyledArea(renderingScope, currentStyle, currentStyleFirstItem, prevItem); | ||
| ctx.beginPath(); | ||
| currentStyle = itemStyle; | ||
| currentStyleFirstItem = currentItem; | ||
| ctx.moveTo(currentX, currentY); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Bug: accumulatedDistance is not reset when breaking at a gap.
When the line breaks at a gap, finishStyledArea is called and a new path begins, but accumulatedDistance (used for dash pattern offset tracking) is not reset. Compare with the changeStyle helper above (lines 52-65), which correctly handles this:
if (shouldTrackDashOffset) {
const offset = accumulatedDistance % dashPatternLength;
ctx.lineDashOffset = offset;
accumulatedDistance = offset;
}This means that when connectGaps: false is combined with a dashed or dotted line style, the dash pattern after a gap will start at the wrong offset rather than resetting cleanly.
Suggested fix: add after ctx.beginPath();:
if (shouldTrackDashOffset) {
accumulatedDistance = 0;
ctx.lineDashOffset = 0;
}| pointMarkersRadius: options.pointMarkersVisible ? (options.pointMarkersRadius || options.lineWidth / 2 + 2) : undefined, | ||
| visibleRange: this._itemsVisibleRange, | ||
| barWidth: this._model.timeScale().barSpacing(), | ||
| connectGaps: options.connectGaps as boolean, |
There was a problem hiding this comment.
The as boolean cast here is a workaround for connectGaps being declared optional in the type interface. If connectGaps is made required in LineStyleOptions (see comment on series-options.ts), this cast should be removed.
Same applies to the casts in area-pane-view.ts (lines 56 and 67) and baseline-pane-view.ts (lines 71 and 85).
There was a problem hiding this comment.
The test covers the connectGaps: false case well for all three series types, but some additional coverage would be valuable:
connectGaps: true(default) with whitespace data — verifying the line connects across gaps (regression test for the default behaviour).- Dashed/dotted line styles with
connectGaps: false— this exercises theaccumulatedDistance/lineDashOffsetcode path which currently has a bug (see comment onwalk-line.ts). LineType.CurvedwithconnectGaps: false— the bezier control point calculation ingetControlPoints()uses adjacent items. After a gap break +moveTo, the first curved segment of the new sub-path might produce unexpected control points sincegetControlPointsstill sees the pre-gap item in the array.- Multiple series with different time ranges and
connectGaps: false— verifying that a series doesn't get spurious gaps from another series' timestamps (this relates to the gap detection logic concern inwalk-line.ts).
|
|
||
| // eslint-disable-next-line max-params, complexity | ||
| export function walkLine<TItem extends LinePoint, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( | ||
| export function walkLine<TItem extends LinePoint & { time?: TimePointIndex }, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( |
There was a problem hiding this comment.
The { time?: TimePointIndex } intersection is unnecessary and over-permissive. Every caller passes items that extend TimedValue (via LineItemBase or AreaFillItemBase), and TimedValue already has time: TimePointIndex as a required property. This optional annotation is what forces the defensive !== undefined checks on line 79.
| export function walkLine<TItem extends LinePoint & { time?: TimePointIndex }, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( | |
| export function walkLine<TItem extends LinePoint & TimedValue, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( |
With TimedValue (already exported from ../model/time-data), the import can stay the same, TimedValue already brings in TimePointIndex. And the gap check on line 79 can be simplified to remove the undefined guards (see comment on that line).
Type of PR: enhancement
PR checklist:
Overview of change:
This PR introduces a new
connectGaps: booleanoption (defaulting totrue) forLine,Area, andBaselineseries.When set to
false, the chart will no longer draw a line connecting data points separated by one or more empty bars (time index distance > 1).Key changes:
walkLinerenderer utility to detect gaps and break path segments.connectGapstoLineStyleOptions,AreaStyleOptions, andBaselineStyleOptions.Is there anything you'd like reviewers to focus on?
Verified that existing charts are unaffected (100+ graphics tests passing). The implementation uses the time index difference to detect gaps as suggested in the original issue discussion.
The image below shows the new connectGaps: false setting in action, correctly breaking the line and area at missing data points.
