Skip to content

Commit

Permalink
Refactor InterpreterControls for readability, debugability, and perfo…
Browse files Browse the repository at this point in the history 
…rmance.

* 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
  • Loading branch information
@RyanMullins
RyanMullins authored and LIT team committed Aug 11, 2023
1 parent fb2467d commit d25392b
Showing 1 changed file with 77 additions and 91 deletions.
168 changes: 77 additions & 91 deletions lit_nlp/client/elements/interpreter_controls.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.
Expand Down Expand Up @@ -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`
<div class="control-holder">
<div class="control-name">
${(required ? '*' : '') + name}
</div>
${this.renderControl(name, spec[name])}
</div>`;
});
return Object.entries(this.spec).map(([fieldName, fieldSpec]) =>
html`<div class="control-holder">
<div class="control-name">
${(fieldSpec.required ? '*' : '') + fieldName}
</div>
${this.renderControl(fieldName, fieldSpec)}
</div>`);
}

renderControl(name: string, controlType: LitType) {
Expand All @@ -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`
<lit-checkbox ?checked=${isSelected} @change=${change}
label=${option} class='checkbox-control'>
</lit-checkbox>
`;
return html`<lit-checkbox ?checked=${isSelected} @change=${change}
label=${option} class='checkbox-control'>
</lit-checkbox>`;
});
return html`<div class='checkbox-holder'>${renderCheckboxes()}</div>`;
} else if (
Expand All @@ -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`
<option value=${optionIndex}>${option}</option>
`;
});
const defaultValue =
vocab != null && vocab.length > 0 ?
vocab[0] :
'';
const defaultValue = vocab[0] || '';
return html`<select class="dropdown control" @change=${updateDropdown}
.value=${defaultValue} ?disabled=${vocab.length < 2}>
${options}
</select>`;
.value=${defaultValue} ?disabled=${vocab.length < 2}>${
vocab.map((option, optionIndex) =>
html`<option value=${optionIndex}>${option}</option>`)
}</select>`;
} 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`
<lit-numeric-input class="slider-holder" min="${minVal}" max="${maxVal}"
step="${step}" value="${+this.settings[name]}"
@change=${updateSettings}></lit-numeric-input>
`;
// clang-format on
return html`<lit-numeric-input class="slider-holder"
min=${minVal} max=${maxVal} step=${step} value=${this.settings[name]}
@change=${updateSettings}></lit-numeric-input>`;
} else if (controlType instanceof BooleanLitType) {
// Render a checkbox.
const toggleVal = () => {
const val = !!this.settings[name];
this.settings[name] = !val;
};
// clang-format off
return html`
<lit-checkbox
?checked=${!!this.settings[name]}
@change=${toggleVal}>
</lit-checkbox>
`;
// clang-format on
const toggleVal = () => {this.settings[name] = !this.settings[name];};
return html`<lit-checkbox ?checked=${!!this.settings[name]}
@change=${toggleVal}></lit-checkbox>`;
} 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`<input class="control" type="text" @input=${updateText}
.value="${value}" />`;
.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`<input class="control" type="text" @input=${updateText}
.value="${value}" />`;
.value=${value} />`;
}
}
}
Expand Down

0 comments on commit d25392b

Please sign in to comment.