Update WeightSlider.js to allow 0 weights (#1005)

This commit #811, allowing users to set the weights of node/edge types to 0.

The WeightSlider now sets the weight to 0 when its dragged to its minimum value.
The logic for converting between weights and sliders has also been made more robust,
and is more thoroughly tested.

In cases where we wanted to set the weight to 0 (e.g. backwards Reaction edges),
the default weight has been changed.

Test plan:
Loading the UI, check that the sliders still work as expected (dragging them changes the displayed weight, dragging to the far left sets weight to 0). Check that the weights are consumed as expected (setting weight for issues to 0 leads to no cred for issues). Check that the weights for backwards reaction edges now have 0 weight. `git grep "TODO(#811)"` returns no hits.

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## [Unreleased]
+- Enable setting type weights to 0 in the UI (#1005)
 - Add support for 🚀 and 👀 reaction types (#1068)
 - Create one page per project, rather than having a selector (#988)
 <!-- Please add new entries to the _top_ of this section. -->

src/explorer/weights/WeightSlider.js

@@ -2,11 +2,55 @@
 
 import React from "react";
 
+/**
+ * The Weight that the user supplies via a WeightSlider.
+ * Weights are finite and non-negative. The vast majority of weights cannot
+ * be selected via the WeightSlider, which is only capable of choosing a few
+ * specific weights.
+ */
+export type Weight = number;
+
+/**
+ * SliderPosition is an integer in the range `[MIN_SLIDER, MAX_SLIDER]` which
+ * describes the current position of the WeightSlider's slider.
+ */
+export type SliderPosition = number;
+
+export const MIN_SLIDER: SliderPosition = -5;
+export const MAX_SLIDER: SliderPosition = 5;
+
 export type Props = {|
-  +weight: number,
+  // The current Weight the WeightSlider is set to
+  // Note: It is acceptable to pass the WeightSlider a weight which the slider
+  // is incapable of picking (e.g. 1/3).
+  +weight: Weight,
+  // The name associated with the slider; displayed adjacent to the slider.
   +name: React$Node,
-  +onChange: (number) => void,
+  // Callback invoked with the new weight after the user adjusts the slider.
+  +onChange: (Weight) => void,
 |};
+
+/**
+ * The WeightSlider is a [controlled component] which lets the user choose a
+ * numeric weight by dragging a slider.
+ *
+ * The weight varies exponentially in the slider position, so that moving the
+ * slider by one notch changes the weight by a power of two. The exception is
+ * when the user drags the slider to its minimum value, in which case the
+ * weight is set to 0.
+ *
+ * In addition to rendering a slider (instantiated as a [range input]), it also renders
+ * a description of the slider, and the weight associated with the current slider position.
+ *
+ * Note that it is possible to configure the WeightSlider with a weight that
+ * that is impossible to select via the slider; for example, 1/3. In such cases,
+ * the WeightSlider's slider will be set to the closest possible position, and after
+ * the user adjusts the weight via the slider, it will always correspond exactly to
+ * the slider position.
+ *
+ * [controlled component]: https://reactjs.org/docs/forms.html#controlled-components
+ * [range input]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range
+ */
 export class WeightSlider extends React.Component<Props> {
   render() {
     return (
@@ -14,13 +58,13 @@ export class WeightSlider extends React.Component<Props> {
         <span style={{flexGrow: 1}}>{this.props.name}</span>
         <input
           type="range"
-          min={-5}
+          min={MIN_SLIDER}
-          max={5}
+          max={MAX_SLIDER}
           step={1}
-          value={Math.log2(this.props.weight)}
+          value={weightToSlider(this.props.weight)}
           onChange={(e) => {
-            const logValue = e.target.valueAsNumber;
+            const weight: Weight = sliderToWeight(e.target.valueAsNumber);
-            this.props.onChange(2 ** logValue);
+            this.props.onChange(weight);
           }}
         />{" "}
         <span
@@ -33,11 +77,65 @@ export class WeightSlider extends React.Component<Props> {
   }
 }
 
+/**
+ * Converts a Weight into a SliderPosition.
+ *
+ * If the Weight is an illegal value (negative, NaN, or infinite), an error
+ * will be thrown.
+ *
+ * It is possible that the Weight will be a legal value, but will not
+ * correspond to any SliderPosition (as SliderPositions are always integers, in
+ * the range `[MIN_SLIDER, MAX_SLIDER]`). In such cases, it may seem principled
+ * to throw a conversion error. However, initial weights are provided by
+ * plugins, and plugin authors might reasonably choose weights like 1/3 that do
+ * not correspond to a slider value. We choose to make the WeightSlider robust
+ * to such decisions by rounding to the closest appropriate slider position. In
+ * such cases, the WeightSlider's weight will be inconsistent with the slider
+ * position, and moving the Slider away from and back to its initial position
+ * will actually change the weight. This is unfortunate, but is preferable to
+ * having the application error.
+ */
+export function weightToSlider(weight: Weight): SliderPosition {
+  if (!isFinite(weight) || weight < 0) {
+    throw new Error(`illegal weight: ${weight}`);
+  }
+  const tentativePosition = Math.log2(weight);
+  if (tentativePosition < MIN_SLIDER) {
+    return MIN_SLIDER;
+  }
+  if (tentativePosition > MAX_SLIDER) {
+    return MAX_SLIDER;
+  }
+  return Math.round(tentativePosition);
+}
+
+/**
+ * Converts a SliderPosition into a Weight.
+ *
+ * SliderPosition must be an integer in the range `[MIN_SLIDER, MAX_SLIDER]`.
+ * If it is not, then an error will be thrown.
+ *
+ * Over the domain of legal slider positions `p`, `weightToSlider(sliderToWeight(p))`
+ * is the identity function.
+ */
+export function sliderToWeight(sliderPosition: SliderPosition): Weight {
+  if (sliderPosition < MIN_SLIDER || sliderPosition > MAX_SLIDER) {
+    throw new Error(`Slider position out of range: ${sliderPosition}`);
+  }
+  if (isNaN(sliderPosition)) {
+    throw new Error("illegal value: NaN");
+  }
+  if (Math.round(sliderPosition) !== sliderPosition) {
+    throw new Error(`slider position not integer: ${sliderPosition}`);
+  }
+  return sliderPosition === MIN_SLIDER ? 0 : 2 ** sliderPosition;
+}
+
 export function formatWeight(n: number) {
-  if (n <= 0 || !isFinite(n)) {
+  if (n < 0 || !isFinite(n)) {
     throw new Error(`Invalid weight: ${n}`);
   }
-  if (n >= 1) {
+  if (n >= 1 || n === 0) {
     return n.toFixed(0) + "×";
   } else {
     return `1/${(1 / n).toFixed(0)}×`;

src/explorer/weights/WeightSlider.test.js

@@ -3,34 +3,50 @@
 import React from "react";
 import {shallow} from "enzyme";
 
-import {WeightSlider, formatWeight} from "./WeightSlider";
+import {
+  WeightSlider,
+  formatWeight,
+  MIN_SLIDER,
+  MAX_SLIDER,
+  sliderToWeight,
+  weightToSlider,
+  type Weight,
+} from "./WeightSlider";
 
 require("../../webutil/testUtil").configureEnzyme();
 
 describe("explorer/weights/WeightSlider", () => {
   describe("WeightSlider", () => {
-    function example() {
+    function example(weight: Weight) {
       const onChange = jest.fn();
       const element = shallow(
-        <WeightSlider weight={3} name="foo" onChange={onChange} />
+        <WeightSlider weight={weight} name="foo" onChange={onChange} />
       );
       return {element, onChange};
     }
-    it("sets slider to the log of provided weight", () => {
+    // These are all valid weights, but not all of them correspond to a valid
-      const {element} = example();
+    // slider position.
-      expect(element.find("input").props().value).toBe(Math.log2(3));
+    const exampleWeights = [0, 2 ** -10, 0.25, 0.33, 1, 20];
+    it("sets the slider as corresponds to the current weight", () => {
+      for (const w of exampleWeights) {
+        const expectedSlider = weightToSlider(w);
+        const {element} = example(w);
+        expect(element.find("input").props().value).toBe(expectedSlider);
+      }
     });
     it("prints the provided weight", () => {
-      const {element} = example();
+      for (const w of exampleWeights) {
+        const {element} = example(w);
         expect(
           element
             .find("span")
             .at(1)
             .text()
-      ).toBe(formatWeight(3));
+        ).toBe(formatWeight(w));
+      }
     });
     it("displays the provided name", () => {
-      const {element} = example();
+      const {element} = example(0);
       expect(
         element
           .find("span")
@@ -38,15 +54,83 @@ describe("explorer/weights/WeightSlider", () => {
           .text()
       ).toBe("foo");
     });
-    it("changes to the slider trigger the onChange with exponentiatied value", () => {
+    it("changes to the slider trigger the onChange with the corresponding weight", () => {
-      const {element, onChange} = example();
+      const sliderVals = [MIN_SLIDER, 0, MAX_SLIDER];
+      for (const sliderVal of sliderVals) {
+        const {element, onChange} = example(0);
         const input = element.find("input");
-      input.simulate("change", {target: {valueAsNumber: 7}});
+        input.simulate("change", {target: {valueAsNumber: sliderVal}});
         expect(onChange).toHaveBeenCalledTimes(1);
-      expect(onChange).toHaveBeenCalledWith(2 ** 7);
+        expect(onChange).toHaveBeenCalledWith(sliderToWeight(sliderVal));
+      }
+    });
+    it("the weight and slider position may be inconsistent", () => {
+      // If the weight does not correspond to an integer slider value, then
+      // changing the slider to its current position can change the weight.
+      // See the docstring on `weightToSlider` for justification.
+      const imperfectWeight = 1 / 3;
+      const {element, onChange} = example(imperfectWeight);
+      const input = element.find("input");
+      const elementSliderValue = input.props().value;
+      input.simulate("change", {target: {valueAsNumber: elementSliderValue}});
+      expect(onChange).not.toHaveBeenCalledWith(imperfectWeight);
     });
   });
 
+  describe("weight<-> slider conversions", () => {
+    it("slider->weight->slider is identity", () => {
+      const legalSliderPositions = [MIN_SLIDER, 0, MAX_SLIDER];
+      for (const sliderPosition of legalSliderPositions) {
+        const weight = sliderToWeight(sliderPosition);
+        const position_ = weightToSlider(weight);
+        expect(sliderPosition).toEqual(position_);
+      }
+    });
+    it("weightToSlider truncates when out of range", () => {
+      const tinyWeight = sliderToWeight(MIN_SLIDER) / 2;
+      expect(weightToSlider(tinyWeight)).toEqual(MIN_SLIDER);
+
+      const giantWeight = sliderToWeight(MAX_SLIDER) * 2;
+      expect(weightToSlider(giantWeight)).toEqual(MAX_SLIDER);
+    });
+    it("weightToSlider errors on invalid weights", () => {
+      const invalid = [NaN, Infinity, -Infinity, -1];
+      for (const v of invalid) {
+        expect(() => weightToSlider(v)).toThrowError("illegal weight");
+      }
+    });
+    it("weightToSlider rounds to closest corresponding slider value", () => {
+      const nonIntegerSliders = [-0.3, 0.3, 0.9];
+      for (const nonIntegerSlider of nonIntegerSliders) {
+        const w = 2 ** nonIntegerSlider;
+        expect(weightToSlider(w)).toEqual(Math.round(nonIntegerSlider));
+      }
+    });
+    it("sliderToWeight errors on slider position out of range", () => {
+      const illegalValues = [
+        -Infinity,
+        MIN_SLIDER - 1,
+        MAX_SLIDER + 1,
+        Infinity,
+      ];
+      for (const illegalValue of illegalValues) {
+        expect(() => sliderToWeight(illegalValue)).toThrowError(
+          "Slider position out of range"
+        );
+      }
+    });
+    it("sliderToWeight errors on non-integer values", () => {
+      const nonIntegers = [-0.3, 0.3, 0.9];
+      for (const nonInteger of nonIntegers) {
+        expect(() => sliderToWeight(nonInteger)).toThrowError(
+          "slider position not integer"
+        );
+      }
+    });
+    it("sliderToWeight errors on NaN", () => {
+      expect(() => sliderToWeight(NaN)).toThrowError("illegal value: NaN");
+    });
+  });
   describe("formatWeight", () => {
     it("shows numbers greater than 1 as a integer-rounded multiplier", () => {
       expect(formatWeight(5.3)).toBe("5×");
@@ -54,8 +138,11 @@ describe("explorer/weights/WeightSlider", () => {
     it("shows numbers less than 1 (but not 0) as integer-rounded fractions", () => {
       expect(formatWeight(0.249)).toBe("1/4×");
     });
+    it("shows numbers equal to 0 as 0x", () => {
+      expect(formatWeight(0)).toBe("0×");
+    });
     it("throws on bad values", () => {
-      const bads = [NaN, Infinity, -Infinity, -3, 0];
+      const bads = [NaN, Infinity, -Infinity, -3];
       for (const bad of bads) {
         expect(() => formatWeight(bad)).toThrowError("Invalid weight");
       }

src/plugins/github/declaration.js

@@ -101,7 +101,7 @@ const referencesEdgeType = Object.freeze({
   forwardName: "references",
   backwardName: "is referenced by",
   defaultForwardWeight: 1,
-  defaultBackwardWeight: 1 / 16,
+  defaultBackwardWeight: 0,
   prefix: E.Prefix.references,
   description: "TODO: Add a description for this EdgeType",
 });
@@ -110,8 +110,7 @@ const mentionsAuthorEdgeType = Object.freeze({
   forwardName: "mentions author of",
   backwardName: "has author mentioned by",
   defaultForwardWeight: 1,
-  // TODO(#811): Probably change this to 0
+  defaultBackwardWeight: 0,
-  defaultBackwardWeight: 1 / 32,
   prefix: E.Prefix.mentionsAuthor,
   description: "TODO: Add a description for this EdgeType",
 });
@@ -120,8 +119,7 @@ const reactsHeartEdgeType = Object.freeze({
   forwardName: "reacted ❤️ to",
   backwardName: "got ❤️ from",
   defaultForwardWeight: 2,
-  // TODO(#811): Probably change this to 0
+  defaultBackwardWeight: 0,
-  defaultBackwardWeight: 1 / 32,
   prefix: E.Prefix.reactsHeart,
   description: "TODO: Add a description for this EdgeType",
 });
@@ -130,8 +128,7 @@ const reactsThumbsUpEdgeType = Object.freeze({
   forwardName: "reacted 👍 to",
   backwardName: "got 👍 from",
   defaultForwardWeight: 1,
-  // TODO(#811): Probably change this to 0
+  defaultBackwardWeight: 0,
-  defaultBackwardWeight: 1 / 32,
   prefix: E.Prefix.reactsThumbsUp,
   description: "TODO: Add a description for this EdgeType",
 });
@@ -140,8 +137,7 @@ const reactsHoorayEdgeType = Object.freeze({
   forwardName: "reacted 🎉 to",
   backwardName: "got 🎉 from",
   defaultForwardWeight: 4,
-  // TODO(#811): Probably change this to 0
+  defaultBackwardWeight: 0,
-  defaultBackwardWeight: 1 / 32,
   prefix: E.Prefix.reactsHooray,
   description: "TODO: Add a description for this EdgeType",
 });
@@ -150,8 +146,7 @@ const reactsRocketEdgeType = Object.freeze({
   forwardName: "reacted 🚀 to",
   backwardName: "got 🚀 from",
   defaultForwardWeight: 1,
-  // TODO(#811): Probably change this to 0
+  defaultBackwardWeight: 0,
-  defaultBackwardWeight: 1 / 32,
   prefix: E.Prefix.reactsRocket,
   description: "TODO: Add a description for this EdgeType",
 });