From 45f59aeafc6fa4c4759073f4cbc401529f05e891 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 16 Oct 2026 13:26:17 +0000 Subject: [PATCH 1/2] Style library: polymers drawn as polymers again, and five smaller fixes From a review of WebsiteAngular's changes since its #373, the version of this library the site now installs from here: - A polymer of a Protein, Molecule or RNA had no offset copy behind it: their background layer is optional (the node's own shape draws the body), and the copy was skipped with it. Every such polymer, and the legend's Polymer key, was drawn as a plain entity since #373. - CROSS was cached on size alone and never cleared, so crossed-out nodes kept the light theme's red after a switch to dark, and two styles on one page shared the first cross; OMMITED_ICON was cached on nothing. Both are keyed on what they draw with, and cleared with the drawers. - The drawing a node gets was remembered by id, classes and selection -- not size or analysis values -- so a node changed without a cache clear kept its old drawing. They are in the key. - interpolate read an exact 0 as "not here" and asserted, and a curve of one point returned 0; ends are inclusive and 0 is an answer. - The gradient cache was keyed on the node id it never uses; nodes with the same values share one. - updateOn 'position style' costs one pass per rendered zoom frame on the molecule layer; kept, and the cost said where it is set. Each behaviour change has a test that failed before it. Co-Authored-By: Claude Opus 5.5 --- dead-code-baseline.json | 2 +- lint-baseline.json | 4 +- .../src/lib/drawer/image-builder.spec.ts | 72 +++++++++++++++++-- .../src/lib/drawer/image-builder.ts | 39 ++++++---- .../src/lib/interactivity.spec.ts | 64 +++++++++++++++++ .../src/lib/interactivity.ts | 14 +++- 6 files changed, 173 insertions(+), 22 deletions(-) create mode 100644 projects/ngx-reactome-cytoscape-style/src/lib/interactivity.spec.ts diff --git a/dead-code-baseline.json b/dead-code-baseline.json index 6c47c18..1d8f83c 100644 --- a/dead-code-baseline.json +++ b/dead-code-baseline.json @@ -1,3 +1,3 @@ { - "total": 18 + "total": 17 } diff --git a/lint-baseline.json b/lint-baseline.json index b9f4dce..1936a7d 100644 --- a/lint-baseline.json +++ b/lint-baseline.json @@ -1,5 +1,5 @@ { - "warnings": 334, + "warnings": 332, "rules": { "@angular-eslint/no-input-rename": 1, "@angular-eslint/prefer-inject": 8, @@ -8,7 +8,7 @@ "@angular-eslint/template/button-has-type": 1, "@typescript-eslint/ban-ts-comment": 22, "@typescript-eslint/no-explicit-any": 9, - "@typescript-eslint/no-non-null-assertion": 56, + "@typescript-eslint/no-non-null-assertion": 54, "@typescript-eslint/no-unsafe-argument": 19, "@typescript-eslint/no-unsafe-assignment": 75, "@typescript-eslint/no-unsafe-call": 51, diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts index f888820..5513a66 100644 --- a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts +++ b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts @@ -2,7 +2,8 @@ import { describe, expect, it } from 'vitest'; import cytoscape from 'cytoscape'; import chroma from 'chroma-js'; import { Style } from '../style'; -import { clearDrawersCache, imageBuilder } from './image-builder'; +import { clearDrawersCache, CROSS, imageBuilder, OMMITED_ICON } from './image-builder'; +import { Properties } from '../properties'; /** * The image builder's caches. @@ -22,15 +23,22 @@ function aStyle() { return style; } -type NodeSpec = { id?: string; width: number; height: number; classes?: string[]; exp?: unknown }; +type NodeSpec = { + id?: string; + width: number; + height: number; + classes?: string[]; + exp?: unknown; + kind?: string; +}; -function aNode({ id = '1', width, height, classes = [], exp }: NodeSpec) { +function aNode({ id = '1', width, height, classes = [], exp, kind = 'Complex' }: NodeSpec) { const cy = cytoscape({ headless: true, elements: [ { data: { id, width, height, displayName: 'X', ...(exp ? { exp } : {}) }, - classes: ['Complex', 'PhysicalEntity', ...classes], + classes: [kind, 'PhysicalEntity', ...classes], }, ], }); @@ -97,6 +105,62 @@ describe('the image builder', () => { expect(polymer.length).toBe(plain.length + shadows.length); }); + // A protein, molecule or RNA draws its body with the node's own shape, so + // its background layer is optional -- never drawn as a layer. The offset + // copy behind a polymer of one was skipped with it, so every such polymer, + // and the legend's Polymer key, looked like a single plain entity (review + // of the changes since WebsiteAngular #373). + for (const kind of ['Protein', 'Molecule', 'RNA']) { + it(`draws a polymer of a ${kind} with its offset copy, the shape once`, () => { + clearDrawersCache(); + const style = aStyle(); + const plain = images(style, aNode({ id: '6', width: 100, height: 40, kind })); + const polymer = images( + style, + aNode({ id: '7', width: 100, height: 40, kind, classes: ['Polymer'] }) + ); + const shadows = polymer.filter((l) => decodeURIComponent(l).includes('style="filter:')); + expect(shadows.length, 'an offset copy').toBeGreaterThan(0); + expect(polymer.length).toBe(plain.length + shadows.length); + }); + } + + // The cross and the omitted icon were cached on size alone (the icon on + // nothing), and the cache was never cleared: after a switch to the dark + // theme, crossed-out nodes kept the light theme's red, and two styles on + // one page shared whichever cross was drawn first. + it('draws a cross in the colour it is asked for, not the first one cached', () => { + clearDrawersCache(); + const with_ = (negative: string) => + ({ global: { negative, thickness: 2, onSurface: negative } }) as unknown as Properties; + const size = { width: 100, height: 40 } as Parameters[1]; + expect(CROSS(with_('#ba1a1a'), size)['background-image']).toContain('#ba1a1a'); + expect(CROSS(with_('#ea2323'), size)['background-image']).toContain('#ea2323'); + expect(OMMITED_ICON(with_('#ba1a1a'))).not.toBe(OMMITED_ICON(with_('#ea2323'))); + }); + + it('forgets every cached cross when the drawers are cleared', () => { + const size = { width: 100, height: 40 } as Parameters[1]; + CROSS({ global: { negative: '#000000', thickness: 2 } } as unknown as Properties, size); + clearDrawersCache(); + expect(CROSS.cache.size).toBe(0); + }); + + // The drawing a node gets is remembered per builder by its id, classes and + // selection -- not its size or analysis values, so a node resized or given + // new values without the cache being cleared kept its old drawing. + it('draws a node again when its size or values change', () => { + clearDrawersCache(); + const build = imageBuilder(aStyle().properties, aStyle()); + const node = aNode({ id: '8', width: 100, height: 40, exp: [0.1] }); + const first = build(node)['background-image']; + node.data('width', 300); + expect(build(node)['background-image']).not.toEqual(first); + const resized = build(node)['background-image']; + node.data('exp', [0.9]); + expect(build(node)['background-image']).not.toEqual(resized); + }); + it('paints the whole node when the same value repeats with counts', () => { clearDrawersCache(); // [value, count] pairs: 2 of 0.3 then 5 more of 0.3 -- one colour across diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts index f1b1552..fe6308f 100644 --- a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts +++ b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts @@ -44,11 +44,14 @@ export const imageBuilder = (properties: Properties, style: Style) => // A polymer is its shape with an offset copy behind it: the copies only. // The shape's own layers are added below like any other node's -- added // here as well, they were drawn twice. + // + // The background is copied even when optional. Optional means the node's + // own shape draws the body, so that layer is never added below -- but + // the copy behind is still this function's to draw. Skipping it left a + // protein, molecule or RNA polymer with no copy at all, drawn as a plain + // entity. if (node.hasClass('Polymer')) { - const drawn = [ - drawer.background && !drawer.background.optional ? drawer.background : undefined, - ...(drawer.decorators || []), - ].filter((layer) => !!layer); + const drawn = [drawer.background, ...(drawer.decorators || [])].filter((layer) => !!layer); for (const originalLayer of drawn) { const polymerLayer = { ...originalLayer }; polymerLayer['background-position-x'] = @@ -120,7 +123,12 @@ export const imageBuilder = (properties: Properties, style: Style) => // console.timeEnd(`build-image-${node.id()}`) return aggregated; }, - (node) => `${node.id()}-${node.classes().toString()}-s:${node.selected()}` + // Everything the drawing depends on: on id, classes and selection alone, a + // node resized or given new analysis values without a cache clear kept the + // drawing it had. + (node) => + `${node.id()}-${node.classes().toString()}-s:${node.selected()}` + + `-${node.data('width')}x${node.data('height')}-e:${JSON.stringify(node.data('exp') ?? null)}` ); const defaultBg: Image = { @@ -232,10 +240,10 @@ function _expToGradient( } /** - * Kept per palette, and keyed on the node's values as well as its id: keyed on - * the id alone it was shared by every diagram on the page, so a node took the - * colours of whichever node with that id was drawn first, and kept them after - * its values changed. + * Kept per palette, and keyed on the node's values: keyed on the id it was + * shared by every diagram on the page, so a node took the colours of whichever + * node with that id was drawn first, and kept them after its values changed. + * The values are all the gradient is drawn from, so nodes sharing them share it. */ let gradients = new WeakMap>(); function expToGradient( @@ -247,7 +255,9 @@ function expToGradient( if (!exps || !palette) return undefined; let cache = gradients.get(palette); if (!cache) gradients.set(palette, (cache = new Map())); - const key = `${id}|${JSON.stringify(exps)}`; + // The values alone: the gradient is drawn from them and the palette, never + // the id, so nodes with the same values share one. + const key = JSON.stringify(exps); if (!cache.has(key)) cache.set(key, _expToGradient(id, exps, properties, palette)); return cache.get(key); } @@ -322,6 +332,7 @@ export function clearDrawersCache() { value.cache.clear(); } OMMITED_ICON.cache.clear(); + CROSS.cache.clear(); } function aggregate( @@ -386,7 +397,10 @@ export const CROSS = memoize( 'background-image-opacity': 1, }; }, - (_p, { width, height }) => `${width}x${height}` + // Keyed on everything it draws with: on size alone, a theme switch kept the + // old theme's colour, and two styles on one page shared the first cross. + (properties, { width, height }) => + `${width}x${height}|${extract(properties.global.negative)}|${extract(properties.global.thickness)}` ); export const OMMITED_ICON = memoize( @@ -398,5 +412,6 @@ export const OMMITED_ICON = memoize( 10 ); }, - (_p) => '' + // Keyed on its colour, for the same reason as CROSS. + (properties) => String(extract(properties.global.onSurface)) ); diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.spec.ts b/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.spec.ts new file mode 100644 index 0000000..e771da6 --- /dev/null +++ b/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.spec.ts @@ -0,0 +1,64 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Interactivity } from './interactivity'; + +/** + * The zoom curves' interpolation. It used `if (y) return y`, so an exact 0 -- + * the curves' own 0-valued points: shadow and trivial opacity at 40%, structure + * opacity at 130% -- read as "not in this segment", ran on to + * console.assert(false), and a curve of one point returned 0, not its value. + */ +const interpolate = (x: number, points: { x: number; y: number }[]) => + Interactivity.prototype.interpolate.call( + { lerp: Interactivity.prototype.lerp } as unknown as Interactivity, + x, + // The curve's points are P, an unexported [x, y] array with x and y + // getters; interpolate reads only those. + points as never + ); + +describe('interpolate', () => { + afterEach(() => vi.restoreAllMocks()); + + it('answers an exact 0 without asserting', () => { + const assert = vi.spyOn(console, 'assert'); + expect( + interpolate(130, [ + { x: 130, y: 0 }, + { x: 150, y: 100 }, + ]) + ).toBe(0); + expect( + interpolate(40, [ + { x: 20, y: 100 }, + { x: 40, y: 0 }, + { x: 60, y: 100 }, + ]) + ).toBe(0); + expect(assert).not.toHaveBeenCalledWith(false, expect.anything()); + }); + + it('gives a curve of one point its value', () => { + expect(interpolate(130, [{ x: 130, y: 50 }])).toBe(50); + }); + + it('interpolates between points as before', () => { + expect( + interpolate(140, [ + { x: 130, y: 0 }, + { x: 150, y: 100 }, + ]) + ).toBe(50); + expect( + interpolate(10, [ + { x: 20, y: 7 }, + { x: 40, y: 9 }, + ]) + ).toBe(7); + expect( + interpolate(90, [ + { x: 20, y: 7 }, + { x: 40, y: 9 }, + ]) + ).toBe(9); + }); +}); diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.ts b/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.ts index 26401d8..fa1d0af 100644 --- a/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.ts +++ b/projects/ngx-reactome-cytoscape-style/src/lib/interactivity.ts @@ -520,6 +520,12 @@ export class Interactivity { selector: '.Molecule', // Not only when a node moves: a node hidden or shown changes no // position, and its structure has to follow it. + // + // Known cost: the zoom handlers write styles to these nodes, so this + // also redoes the layer once per rendered zoom frame (cytoscape-layers + // batches the events per render). It only shows on diagrams with many + // molecules; catching show and hide alone would need every place that + // hides a node to say so, which this does not depend on. updateOn: 'position style', queryEachTime: false, } @@ -725,11 +731,13 @@ export class Interactivity { } interpolate(x: number, points: P[]): number { - if (x < points.at(0)!.x) return points.at(0)!.y; - if (x > points.at(-1)!.x) return points.at(-1)!.y; + // Ends inclusive, so a curve of one point is that point's value. + if (x <= points.at(0)!.x) return points.at(0)!.y; + if (x >= points.at(-1)!.x) return points.at(-1)!.y; for (let i = 0; i + 1 < points.length; i++) { const y = this.lerp(x, points[i], points[i + 1]); - if (y) return y; + // An exact 0 is an answer, as the curves' own 0-valued points are. + if (y !== undefined) return y; } console.assert(false, 'Should not arrive here'); return 0; From 514e98595492812d66ddcbddf84ec9d3950a8410 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 16 Oct 2026 13:31:26 +0000 Subject: [PATCH 2/2] Review of #21: paint a polymer's optional body again over its copy Cytoscape paints every background layer over the node's own body, so the polymer copy of an optional background (protein, molecule, RNA) was drawn over the body -- shifted and darkened -- not behind it. Before WebsiteAngular #373 the original was pushed again after the copies, which is what made the copy read as behind; #373 dropped that with the double-drawn decorators. The optional background goes in once more after the copies; decorators still once. The test now checks that order, and failed before. Co-Authored-By: Claude Opus 5.5 --- .../src/lib/drawer/image-builder.spec.ts | 13 ++++++++++++- .../src/lib/drawer/image-builder.ts | 7 +++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts index 5513a66..6fe9b94 100644 --- a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts +++ b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.spec.ts @@ -121,7 +121,18 @@ describe('the image builder', () => { ); const shadows = polymer.filter((l) => decodeURIComponent(l).includes('style="filter:')); expect(shadows.length, 'an offset copy').toBeGreaterThan(0); - expect(polymer.length).toBe(plain.length + shadows.length); + // Cytoscape paints every layer over the node's own body, so the copy can + // only look as if it is behind if the body's own shape is painted again + // over it: the original background, right after the copy -- once. + expect(polymer.length).toBe(plain.length + shadows.length + 1); + const filtered = polymer.map((l) => decodeURIComponent(l).includes('style="filter:')); + const lastShadow = filtered.lastIndexOf(true); + const unfiltered = polymer.filter((l) => !decodeURIComponent(l).includes('style="filter:')); + expect(lastShadow, 'the original comes after the copy').toBeLessThan(polymer.length - 1); + expect( + unfiltered.filter((l) => l === polymer[lastShadow + 1]).length, + 'and is drawn once' + ).toBe(1); }); } diff --git a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts index fe6308f..674ac45 100644 --- a/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts +++ b/projects/ngx-reactome-cytoscape-style/src/lib/drawer/image-builder.ts @@ -47,9 +47,11 @@ export const imageBuilder = (properties: Properties, style: Style) => // // The background is copied even when optional. Optional means the node's // own shape draws the body, so that layer is never added below -- but - // the copy behind is still this function's to draw. Skipping it left a + // the polymer's copy is still this function's to draw. Skipping it left a // protein, molecule or RNA polymer with no copy at all, drawn as a plain - // entity. + // entity. And since cytoscape paints every layer over the node's body, + // the copy only reads as behind it if the body's shape is painted again + // over the copy: an optional background goes in once more after it. if (node.hasClass('Polymer')) { const drawn = [drawer.background, ...(drawer.decorators || [])].filter((layer) => !!layer); for (const originalLayer of drawn) { @@ -69,6 +71,7 @@ export const imageBuilder = (properties: Properties, style: Style) => ''; layers.push(polymerLayer); } + if (drawer.background?.optional) layers.push(drawer.background); } if (node.hasClass('flag') && drawer.flag) layers.push(drawer.flag);