Style library: polymers drawn as polymers again, and five smaller fixes

dead-code-baseline.json

@@ -1,3 +1,3 @@
 {
-  "total": 18
+  "total": 17
 }

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,

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<typeof CROSS>[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<typeof CROSS>[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

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) =>
             '</g>';
           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<chroma.Scale, Map<string, string | undefined>>();
 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<string, string | undefined>()));
-  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<T extends object, K extends keyof T>(
@@ -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))
 );

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);
+  });
+});

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;