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..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 @@ -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,73 @@ 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); + // 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); + }); + } + + // 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..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 @@ -44,11 +44,16 @@ 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 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. 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.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'] = @@ -66,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); @@ -120,7 +126,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 +243,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 +258,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 +335,7 @@ export function clearDrawersCache() { value.cache.clear(); } OMMITED_ICON.cache.clear(); + CROSS.cache.clear(); } function aggregate( @@ -386,7 +400,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 +415,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;