diff --git a/docs/IMPLEMENTATION_PLAN.md b/docs/IMPLEMENTATION_PLAN.md index 3e224cf..b5e0056 100644 --- a/docs/IMPLEMENTATION_PLAN.md +++ b/docs/IMPLEMENTATION_PLAN.md @@ -1578,3 +1578,25 @@ Tests clean up after themselves: `withChart` records the drawings that exist before the body runs and deletes anything new afterwards, because the dev store is shared with whoever is using the app. Select by title rather than class when asserting on chart overlays, for the same reason. + +### The snapping rule, stated once + +**x picks the bar, y picks which extreme.** That is the whole rule. It is +written here because changing it reactively three times is what made trendlines +feel broken, not any inherent difficulty: + +1. An 8px proximity gate meant a cursor between the high and the low snapped to + neither, so the anchor kept a raw mid-bar price and the side silently fell + back to the dropdown. +2. Removing the gate fixed that. Then a nearest-in-2D search was tried, to make + a bar easier to hit when zoomed out — and broke sweeping along the bottom, + because whichever nearby bar had the lowest low won on total distance and the + dot skipped off the bar under the cursor. Reverted. +3. What actually made it feel wrong was never the rule: the crosshair was in + Lightweight Charts' default Magnet mode, snapping to the bar's *close*, so + the feedback pointed somewhere the anchor would never go. It is Normal + everywhere now, with the snap dot showing the real target. + +An e2e test sweeps the cursor along the bottom of a zoomed-out 1h chart and +requires every position to land on the low of the bar beneath it — 115 of 115. +That test is the rule, executable. diff --git a/static/chart.js b/static/chart.js index ea69fd3..1d6dcc4 100644 --- a/static/chart.js +++ b/static/chart.js @@ -47,10 +47,6 @@ class ConfluenceChart { // rather than a click. Wide enough to survive a twitch on a deliberate click. static DRAG_THRESHOLD = 12; - // Bars either side of the cursor considered when snapping. Wide enough to - // catch the neighbour you meant when bars are three pixels apart. - static SNAP_NEIGHBOURS = 6; - static snapToTick(price) { return Math.round(price / ConfluenceChart.TICK) * ConfluenceChart.TICK; } @@ -98,6 +94,11 @@ class ConfluenceChart { rightPriceScale: { borderVisible: false }, // Daily context lives on the left, intraday on the right. leftPriceScale: { visible: true, borderVisible: false }, + // Never magnet. The default snaps the crosshair to the bar's *close*, so + // hovering beside a low reads a price several ticks away, and everything + // that snaps in this app snaps to extremes. The crosshair tracks the + // cursor; the snap dot says where an anchor would actually land. + crosshair: { mode: LightweightCharts.CrosshairMode.Normal }, }); this.candles = this.chart.addSeries(LightweightCharts.CandlestickSeries, { upColor: '#27825c', downColor: '#bd4545', borderVisible: true, @@ -540,17 +541,7 @@ class ConfluenceChart { */ armTool(tool) { this.armedTool = tool; - // Magnet snaps the crosshair to the bar's close, so hovering by a bar's low - // drew it mid-bar and made a correctly-placed anchor look wrong. While a - // tool is armed the crosshair tracks the cursor and the snap dot shows - // where the anchor will actually land. - this.chart.applyOptions({ - handleScroll: !tool, - handleScale: !tool, - crosshair: { - mode: tool ? LightweightCharts.CrosshairMode.Normal : LightweightCharts.CrosshairMode.Magnet, - }, - }); + this.chart.applyOptions({ handleScroll: !tool, handleScale: !tool }); this.hideSnapDot(); this.chartEl.classList.toggle('armed', Boolean(tool)); this.pendingAnchor = null; @@ -687,40 +678,27 @@ class ConfluenceChart { if (!this.snapToBars || !this.bars.length || fallbackT == null) return base; // An already-snapped point carries no cursor position; return it untouched // rather than measuring against undefined. - if (point.y == null || point.x == null) return base; + if (point.y == null) return base; - // Nearest extreme by *screen* distance, across a few bars either side — - // not the extreme of whichever bar happens to share the cursor's time. - // Taking the bar by time alone meant pointing anywhere below a candle - // snapped to that candle's low however far away it was, while the extreme - // actually under the cursor was ignored. Zoomed out to ~360 bars at three - // pixels each, that made hitting the intended bar a matter of several - // tries. - let lo = 0; - let hi = this.bars.length - 1; - while (lo < hi) { - const mid = (lo + hi + 1) >> 1; - if (this.bars[mid].t <= fallbackT) lo = mid; - else hi = mid - 1; - } - const timeScale = this.chart.timeScale(); - let best = null; - const from = Math.max(0, lo - ConfluenceChart.SNAP_NEIGHBOURS); - const to = Math.min(this.bars.length - 1, lo + ConfluenceChart.SNAP_NEIGHBOURS); - for (let index = from; index <= to; index += 1) { - const bar = this.bars[index]; - const x = timeScale.timeToCoordinate(bar.t); - if (x == null) continue; - for (const [price, side] of [[bar.h, 'resistance'], [bar.l, 'support']]) { - const y = this.candles.priceToCoordinate(price); - if (y == null) continue; - const distance = Math.hypot(x - point.x, y - point.y); - if (!best || distance < best.distance) best = { distance, t: bar.t, p: price, side }; - } - } - return best ? { t: best.t, p: best.p, snappedSide: best.side } : base; + // Horizontal position chooses the bar, vertical position chooses which of + // its extremes. Nothing else — a nearest-in-2D search was tried and is + // wrong: sweeping along the bottom of the chart, whichever nearby bar had + // the lowest low won on total distance, so the dot skipped off the bar + // under the cursor entirely instead of tracing each bar's low in turn. + const nearest = this.bars.reduce( + (best, bar) => (Math.abs(bar.t - fallbackT) < Math.abs(best.t - fallbackT) ? bar : best), + ); + const candidates = [ + { p: nearest.h, side: 'resistance' }, + { p: nearest.l, side: 'support' }, + ]; + const snapped = candidates + .map(value => ({ ...value, distance: Math.abs(this.candles.priceToCoordinate(value.p) - point.y) })) + .sort((a, b) => a.distance - b.distance)[0]; + return { t: nearest.t, p: snapped.p, snappedSide: snapped.side }; } + renderGesture() { if (!this.gesture) return; const { start, end } = this.gesture; diff --git a/tests/e2e/trendline.test.mjs b/tests/e2e/trendline.test.mjs index e832614..8a66f52 100644 --- a/tests/e2e/trendline.test.mjs +++ b/tests/e2e/trendline.test.mjs @@ -125,14 +125,12 @@ test('hovering shows where the anchor will land', { timeout: 180000 }, async () }); }); -test('snapping picks the extreme nearest on screen, not the bar under the cursor time', +test('sweeping the bottom traces the low of each bar under the cursor', { timeout: 180000 }, async () => { await withChart(async page => { await page.click('.timeframes button:text-is("1h")'); await page.waitForTimeout(3500); - // Zoomed out, bars are a few pixels apart and the intended one is hard to - // hit. Snapping used to take whichever bar shared the cursor's *time* and - // then its nearer extreme, ignoring how far away that was. + // Zoomed out to a few pixels per bar, which is where this fell apart. await page.evaluate(() => { const c = window.__chart, ts = c.chart.timeScale(); ts.setVisibleRange({ @@ -142,23 +140,41 @@ test('snapping picks the extreme nearest on screen, not the bar under the cursor }); await page.waitForTimeout(1500); + // The rule, stated once: x picks the bar, y picks which extreme. Running + // the cursor along the bottom must therefore trace each bar's low. A + // nearest-in-2D search was tried instead and broke exactly this — the + // lowest low nearby won on total distance and the dot skipped off the bar + // under the cursor. const result = await page.evaluate(() => { const c = window.__chart, ts = c.chart.timeScale(); - const target = c.bars[c.bars.length - 150]; - const x = ts.timeToCoordinate(target.t); - const y = c.candles.priceToCoordinate(target.l); - // Sit exactly on the target's low but nudged so the cursor's time - // resolves to its neighbour. - const point = { x: x - 2, y, p: c.candles.coordinateToPrice(y), t: Number(ts.coordinateToTime(x - 2)) }; - const snapped = c.snapPoint(point); - return { - resolvedToNeighbour: point.t !== target.t, - snappedToTarget: snapped.t === target.t && snapped.p === target.l, - }; + const y = document.querySelector('#chart').clientHeight * 0.93; + let hits = 0, total = 0; + const misses = []; + for (let x = 200; x < 1000; x += 7) { + const t = ts.coordinateToTime(x); + if (t == null) continue; + const under = c.bars.reduce( + (best, bar) => (Math.abs(bar.t - Number(t)) < Math.abs(best.t - Number(t)) ? bar : best)); + const snapped = c.snapPoint({ x, y, p: c.candles.coordinateToPrice(y), t: Number(t) }); + total += 1; + if (snapped.t === under.t && snapped.p === under.l) hits += 1; + else if (misses.length < 3) misses.push({ x, wanted: under.l, got: snapped.p }); + } + return { hits, total, misses }; }); - assert.equal(result.resolvedToNeighbour, true, 'the probe did not straddle two bars'); - assert.equal(result.snappedToTarget, true, - 'snapped to the bar sharing the cursor time rather than the extreme under the cursor'); + assert.ok(result.total > 50, 'the sweep did not probe enough positions'); + assert.equal(result.hits, result.total, + `only ${result.hits}/${result.total} landed on the low of the bar under the cursor: ` + + JSON.stringify(result.misses)); assertNoPageErrors(page, assert); }); }); + +test('the crosshair never magnets to the close', { timeout: 180000 }, async () => { + await withChart(async page => { + // Magnet mode snaps the crosshair to the bar's close, so hovering beside a + // low reads a price several ticks from the one that would be used. + const mode = await page.evaluate(() => window.__chart.chart.options().crosshair.mode); + assert.equal(mode, 0, 'crosshair is not in Normal mode'); + }); +});