From 71c1d83503ab51274aa9d3aa4a5d3dcbbecbc622 Mon Sep 17 00:00:00 2001 From: Mark Silverwood Date: Mon, 2 Oct 2023 12:52:21 +0100 Subject: [PATCH] don't shift chart when new data replaces existing whitespace (#1431) * don't shift chart when new data replaces existing whitespace * add notes to e2e tests about expected result --- src/model/chart-model.ts | 12 ++++- src/model/time-scale.ts | 14 +++--- .../histogram-add-new-color-on-update.js | 5 ++- .../test-cases/series/whitespace-updates.js | 3 ++ ...t-shift-range-when-replacing-whitespace.js | 44 +++++++++++++++++++ 5 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 tests/e2e/graphics/test-cases/time-scale/do-not-shift-range-when-replacing-whitespace.js diff --git a/src/model/chart-model.ts b/src/model/chart-model.ts index fda1a32534..9a46cdc51a 100644 --- a/src/model/chart-model.ts +++ b/src/model/chart-model.ts @@ -779,6 +779,7 @@ export class ChartModel implements IDestroyable, IChartModelBase public updateTimeScale(newBaseIndex: TimePointIndex | null, newPoints?: readonly TimeScalePoint[], firstChangedPointIndex?: number): void { const oldFirstTime = this._timeScale.indexToTime(0 as TimePointIndex); + const oldLastIndex = this._timeScale.lastIndex(); if (newPoints !== undefined && firstChangedPointIndex !== undefined) { this._timeScale.update(newPoints, firstChangedPointIndex); } @@ -797,7 +798,16 @@ export class ChartModel implements IDestroyable, IChartModelBase const isSeriesPointsAdded = newBaseIndex !== null && newBaseIndex > currentBaseIndex; const isSeriesPointsAddedToRight = isSeriesPointsAdded && !isLeftBarShiftToLeft; - const needShiftVisibleRangeOnNewBar = isLastSeriesBarVisible && this._timeScale.options().shiftVisibleRangeOnNewBar; + // If the lastIndex of the time scale hasn't changed then this means + // that the new point/s has been placed on existing time index point/s. + // This can happen when you have a whitespace series which extends past + // the baseIndex, and now you are adding a new data point at one of those + // whitespace locations. In this case we would not want the chart to + // shift the visible range. #1201 + const lastIndex = this._timeScale.lastIndex(); + const replacedExistingWhitespace = lastIndex === oldLastIndex; + + const needShiftVisibleRangeOnNewBar = isLastSeriesBarVisible && !replacedExistingWhitespace && this._timeScale.options().shiftVisibleRangeOnNewBar; if (isSeriesPointsAddedToRight && !needShiftVisibleRangeOnNewBar) { const compensationShift = newBaseIndex - currentBaseIndex; this._timeScale.setRightOffset(this._timeScale.rightOffset() - compensationShift); diff --git a/src/model/time-scale.ts b/src/model/time-scale.ts index bad11b1814..4fde149d35 100644 --- a/src/model/time-scale.ts +++ b/src/model/time-scale.ts @@ -360,7 +360,7 @@ export class TimeScale implements ITimeScale { const to = Math.round(range.to); const firstIndex = ensureNotNull(this._firstIndex()); - const lastIndex = ensureNotNull(this._lastIndex()); + const lastIndex = ensureNotNull(this.lastIndex()); return { from: ensureNotNull(this.indexToTimeScalePoint(Math.max(firstIndex, from) as TimePointIndex)), @@ -506,7 +506,7 @@ export class TimeScale implements ITimeScale { const earliestIndexOfSecondLabel = (this._firstIndex() as number) + indexPerLabel; // according to indexPerLabel value this value means "earliest index which _might be_ used as the second last label on time scale" - const indexOfSecondLastLabel = (this._lastIndex() as number) - indexPerLabel; + const indexOfSecondLastLabel = (this.lastIndex() as number) - indexPerLabel; const isAllScalingAndScrollingDisabled = this._isAllScalingAndScrollingDisabled(); const isLeftEdgeFixed = this._options.fixLeftEdge || isAllScalingAndScrollingDisabled; @@ -734,7 +734,7 @@ export class TimeScale implements ITimeScale { public fitContent(): void { const first = this._firstIndex(); - const last = this._lastIndex(); + const last = this.lastIndex(); if (first === null || last === null) { return; } @@ -758,6 +758,10 @@ export class TimeScale implements ITimeScale { return this._horzScaleBehavior.formatHorzItem(timeScalePoint.time); } + public lastIndex(): TimePointIndex | null { + return this._points.length === 0 ? null : (this._points.length - 1) as TimePointIndex; + } + private _isAllScalingAndScrollingDisabled(): boolean { const { handleScroll, handleScale } = this._model.options(); return !handleScroll.horzTouchDrag @@ -774,10 +778,6 @@ export class TimeScale implements ITimeScale { return this._points.length === 0 ? null : 0 as TimePointIndex; } - private _lastIndex(): TimePointIndex | null { - return this._points.length === 0 ? null : (this._points.length - 1) as TimePointIndex; - } - private _rightOffsetForCoordinate(x: Coordinate): number { return (this._width - 1 - x) / this._barSpacing; } diff --git a/tests/e2e/graphics/test-cases/series/histogram-add-new-color-on-update.js b/tests/e2e/graphics/test-cases/series/histogram-add-new-color-on-update.js index ed03319f82..039f9f9a7b 100644 --- a/tests/e2e/graphics/test-cases/series/histogram-add-new-color-on-update.js +++ b/tests/e2e/graphics/test-cases/series/histogram-add-new-color-on-update.js @@ -1,3 +1,7 @@ +/* + End result should have a purple histogram bar (value of 499) as + the last visible bar. +*/ function generateData() { const colors = [ '#013370', @@ -23,7 +27,6 @@ function runTestCase(container) { const chart = window.chart = LightweightCharts.createChart(container); const mainSeries = chart.addHistogramSeries({ - lineWidth: 1, color: '#ff0000', }); diff --git a/tests/e2e/graphics/test-cases/series/whitespace-updates.js b/tests/e2e/graphics/test-cases/series/whitespace-updates.js index c21ebe8473..9359af26ac 100644 --- a/tests/e2e/graphics/test-cases/series/whitespace-updates.js +++ b/tests/e2e/graphics/test-cases/series/whitespace-updates.js @@ -1,3 +1,6 @@ +/* + End result should be 4 visible bars, 1 whitespace, and finally 1 visible bar. +*/ function runTestCase(container) { const chart = window.chart = LightweightCharts.createChart(container); diff --git a/tests/e2e/graphics/test-cases/time-scale/do-not-shift-range-when-replacing-whitespace.js b/tests/e2e/graphics/test-cases/time-scale/do-not-shift-range-when-replacing-whitespace.js new file mode 100644 index 0000000000..d8b2a79cff --- /dev/null +++ b/tests/e2e/graphics/test-cases/time-scale/do-not-shift-range-when-replacing-whitespace.js @@ -0,0 +1,44 @@ +const seriesOneData = [ + { time: '2019-04-11', value: 80.01 }, + { time: '2019-04-12', value: 96.63 }, + { time: '2019-04-13', value: 76.64 }, + { time: '2019-04-14', value: 81.89 }, + { time: '2019-04-15', value: 74.43 }, + { time: '2019-04-16', value: 80.01 }, +]; + +const seriesTwoWhitespaceData = [ + { time: '2019-04-17' }, + { time: '2019-04-18' }, + { time: '2019-04-19' }, + { time: '2019-04-20' }, +]; + +function runTestCase(container) { + const chart = window.chart = LightweightCharts.createChart(container, { + timeScale: { + barSpacing: 30, + rightOffset: 10, + shiftVisibleRangeOnNewBar: true, + shiftVisibleRangeWhenNewBarReplacesWhitespace: false, + }, + }); + + const s1 = chart.addLineSeries({ + color: 'red', + }); + s1.setData(seriesOneData); + + return new Promise(resolve => { + requestAnimationFrame(() => { + const s2 = chart.addLineSeries({ + color: 'blue', + }); + s2.setData(seriesTwoWhitespaceData); + requestAnimationFrame(() => { + s1.update({ time: '2019-04-17', value: 84.43 }); + requestAnimationFrame(resolve); + }); + }); + }); +}