From d25392bb64ff0d0fddd481a9651526c483d27ab3 Mon Sep 17 00:00:00 2001 From: Ryan Mullins Date: Fri, 11 Aug 2023 04:55:23 -0700 Subject: [PATCH] Refactor InterpreterControls for readability, debugability, and performance. * Disassociates @observable and @property() decorators where appropriate. * Computes the defaultSettings when the Spec changes. * Resets the settings when the defaultSettings. * Removes any code that changed a settings property in the render() loop. * Refactors the rendering code to fallback to defaultSettings if the value is settings is undefined. * Refactors settings assignment code for stability. PiperOrigin-RevId: 555902678 --- .../client/elements/interpreter_controls.ts | 168 ++++++++---------- 1 file changed, 77 insertions(+), 91 deletions(-) diff --git a/lit_nlp/client/elements/interpreter_controls.ts b/lit_nlp/client/elements/interpreter_controls.ts index f80ebca3..8a548b18 100644 --- a/lit_nlp/client/elements/interpreter_controls.ts +++ b/lit_nlp/client/elements/interpreter_controls.ts @@ -21,7 +21,7 @@ import '../elements/numeric_input'; import {html} from 'lit'; import {customElement, property} from 'lit/decorators.js'; -import {observable} from 'mobx'; +import {computed, observable} from 'mobx'; import {ReactiveElement} from '../lib/elements'; import {BooleanLitType, CategoryLabel, LitType, LitTypeWithVocab, MultiFieldMatcher, Scalar, SingleFieldMatcher, SparseMultilabel, Tokens} from '../lib/lit_types'; @@ -47,19 +47,57 @@ export interface InterpreterClick { */ @customElement('lit-interpreter-controls') export class InterpreterControls extends ReactiveElement { - @observable @property({type: Object}) spec = {}; - @observable @property({type: String}) name = ''; - @observable @property({type: String}) description = ''; - @observable @property({type: String}) applyButtonText = 'Apply'; + @observable.struct @property({type: Object}) spec: Spec = {}; + + @property({type: String}) name = ''; + @property({type: String}) description = ''; + @property({type: String}) applyButtonText = 'Apply'; @property({type: Boolean, reflect: true}) applyButtonDisabled = false; - @observable settings: InterpreterSettings = {}; @property({type: Boolean, reflect: true}) noexpand = false; @property({type: Boolean, reflect: true}) opened = false; + private settings: InterpreterSettings = {}; + static override get styles() { return [sharedStyles, styles]; } + // TODO(b/295346190): Extract this to utils.ts, add tests, use across the app. + @computed get defaultSettings(): InterpreterSettings { + const settings: InterpreterSettings = {}; + for (const [fieldName, fieldSpec] of Object.entries(this.spec)) { + if (fieldSpec instanceof MultiFieldMatcher) { + // If select all is True, default value is all of vocab. + settings[fieldName] = fieldSpec.select_all ? + (fieldSpec.vocab || []) : fieldSpec.default; + } else if ( + fieldSpec instanceof CategoryLabel || + fieldSpec instanceof SingleFieldMatcher) { + // SingleFieldMatcher has its vocab set outside of this element. + const {vocab} = fieldSpec as LitTypeWithVocab; + if (vocab == null) { + settings[fieldName] = ''; + } else { + const [first] = vocab; + settings[fieldName] = first || ''; + } + } else if (fieldSpec instanceof SparseMultilabel) { + settings[fieldName] = fieldSpec.default || []; + } else { + settings[fieldName] = fieldSpec.default as string; + } + } + return settings; + } + + override connectedCallback() { + super.connectedCallback(); + this.settings = structuredClone(this.defaultSettings); + this.react( + () => this.spec, + () => {this.settings = structuredClone(this.defaultSettings);}); + } + override render() { const apply = () => { // Event to be dispatched when an interpreter is applied. @@ -104,40 +142,13 @@ export class InterpreterControls extends ReactiveElement { } renderControls() { - const spec = this.spec as Spec; - return Object.keys(spec).map(name => { - // Ensure a default value for any of the options provided for setting. - if (this.settings[name] == null) { - if (spec[name] instanceof SparseMultilabel) { - this.settings[name] = (spec[name] as SparseMultilabel).default; - } - // If select all is True, default value is all of vocab. - if (spec[name] instanceof MultiFieldMatcher) { - const fieldSpec = spec[name] as MultiFieldMatcher; - this.settings[name] = fieldSpec.select_all ? - fieldSpec.vocab as string[] : - fieldSpec.default; - } - // SingleFieldMatcher has its vocab set outside of this element. - else if ( - spec[name] instanceof CategoryLabel || - spec[name] instanceof SingleFieldMatcher) { - const {vocab} = spec[name] as LitTypeWithVocab; - this.settings[name] = - vocab != null && vocab.length > 0 ? vocab[0] : ''; - } else { - this.settings[name] = spec[name].default as string; - } - } - const required = spec[name].required; - return html` -
-
- ${(required ? '*' : '') + name} -
- ${this.renderControl(name, spec[name])} -
`; - }); + return Object.entries(this.spec).map(([fieldName, fieldSpec]) => + html`
+
+ ${(fieldSpec.required ? '*' : '') + fieldName} +
+ ${this.renderControl(fieldName, fieldSpec)} +
`); } renderControl(name: string, controlType: LitType) { @@ -151,22 +162,19 @@ export class InterpreterControls extends ReactiveElement { } // Render checkboxes, with the first item selected. const renderCheckboxes = () => vocab.map(option => { - // tslint:disable-next-line:no-any - const change = (e: any) => { - if (e.target.checked) { + const change = (e: Event) => { + if ((e.target as HTMLInputElement).checked) { (this.settings[name] as string[]).push(option); } else { - this.settings[name] = (this.settings[name] as string[]) - .filter(item => item !== option); + this.settings[name] = + (this.settings[name] as string[]).filter(e => e !== option); } }; const isSelected = (this.settings[name] as string[]).indexOf(option) !== -1; - return html` - - - `; + return html` + `; }); return html`
${renderCheckboxes()}
`; } else if ( @@ -180,70 +188,48 @@ export class InterpreterControls extends ReactiveElement { } // Render a dropdown, with the first item selected. const updateDropdown = (e: Event) => { - const select = (e.target as HTMLSelectElement); - this.settings[name] = vocab[select?.selectedIndex || 0]; + const index = (e.target as HTMLSelectElement).selectedIndex || 0; + this.settings[name] = vocab[index]; }; - const options = vocab.map((option, optionIndex) => { - return html` - - `; - }); - const defaultValue = - vocab != null && vocab.length > 0 ? - vocab[0] : - ''; + const defaultValue = vocab[0] || ''; return html``; + .value=${defaultValue} ?disabled=${vocab.length < 2}>${ + vocab.map((option, optionIndex) => + html``) + }`; } else if (controlType instanceof Scalar) { // Render a slider. const {step, min_val: minVal, max_val: maxVal} = controlType; const updateSettings = (e: Event) => { - const {value} = (e.target as HTMLInputElement); - this.settings[name] = value; + this.settings[name] = (e.target as HTMLInputElement).value; }; - // clang-format off - return html` - - `; - // clang-format on + return html``; } else if (controlType instanceof BooleanLitType) { // Render a checkbox. - const toggleVal = () => { - const val = !!this.settings[name]; - this.settings[name] = !val; - }; - // clang-format off - return html` - - - `; - // clang-format on + const toggleVal = () => {this.settings[name] = !this.settings[name];}; + return html``; } else if (controlType instanceof Tokens) { // Render a text input box and split on commas. - const value = this.settings[name] as string || ''; + const value = (this.settings[name] as string) || ''; const updateText = (e: Event) => { const input = e.target! as HTMLInputElement; this.settings[name] = input.value.split(',').map(val => val.trim()); }; return html``; + .value=${value} />`; } else { // Render a text input box. - const value = this.settings[name] as string || ''; + const value = (this.settings[name] as string) || ''; const updateText = (e: Event) => { - const input = e.target! as HTMLInputElement; - this.settings[name] = input.value; + this.settings[name] = (e.target as HTMLInputElement).value; }; return html``; + .value=${value} />`; } } }