Clean up modal/options dialog disposable use (#9739)

* Fix duplicate options and clean up disposable use

* Remove unneeded import

* Undo options fix - separating out in different PR
This commit is contained in:
Charles Gagnon
2020-03-26 14:46:22 -07:00
committed by GitHub
parent fd950391e4
commit 68510d54cb
3 changed files with 18 additions and 24 deletions

View File

@@ -7,7 +7,7 @@ import { attachButtonStyler } from 'vs/platform/theme/common/styler';
import { Color } from 'vs/base/common/color';
import { KeyCode, KeyMod } from 'vs/base/common/keyCodes';
import { mixin } from 'vs/base/common/objects';
import { Disposable, IDisposable } from 'vs/base/common/lifecycle';
import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
import * as DOM from 'vs/base/browser/dom';
import { StandardKeyboardEvent } from 'vs/base/browser/keyboardEvent';
import { generateUuid } from 'vs/base/common/uuid';
@@ -83,6 +83,7 @@ export abstract class Modal extends Disposable implements IThemable {
protected _useDefaultMessageBoxLocation: boolean = true;
protected _messageElement: HTMLElement;
protected _modalOptions: IModalOptions;
protected readonly disposableStore = this._register(new DisposableStore());
private _detailsButtonContainer: HTMLElement;
private _messageIcon: HTMLElement;
private _messageSeverity: HTMLElement;
@@ -117,10 +118,6 @@ export abstract class Modal extends Disposable implements IThemable {
private _leftFooter: HTMLElement;
private _rightFooter: HTMLElement;
private _footerButtons: Button[];
private _keydownListener: IDisposable;
private _resizeListener: IDisposable;
private _backButton: Button;
private _modalShowingContext: IContextKey<Array<string>>;
@@ -359,7 +356,7 @@ export abstract class Modal extends Disposable implements IThemable {
DOM.append(this.layoutService.container, this._bodyContainer);
this.setInitialFocusedElement();
this._keydownListener = DOM.addDisposableListener(document, DOM.EventType.KEY_DOWN, (e: KeyboardEvent) => {
this.disposableStore.add(DOM.addDisposableListener(document, DOM.EventType.KEY_DOWN, (e: KeyboardEvent) => {
let context = this._modalShowingContext.get()!;
if (context[context.length - 1] === this._staticKey) {
let event = new StandardKeyboardEvent(e);
@@ -373,10 +370,10 @@ export abstract class Modal extends Disposable implements IThemable {
this.handleForwardTab(e);
}
}
});
this._resizeListener = DOM.addDisposableListener(window, DOM.EventType.RESIZE, (e: Event) => {
}));
this.disposableStore.add(DOM.addDisposableListener(window, DOM.EventType.RESIZE, (e: Event) => {
this.layout(DOM.getTotalHeight(this._modalBodySection));
});
}));
this.layout(DOM.getTotalHeight(this._modalBodySection));
this._telemetryService.createActionEvent(TelemetryKeys.TelemetryView.Shell, TelemetryKeys.ModalDialogOpened)
@@ -395,8 +392,7 @@ export abstract class Modal extends Disposable implements IThemable {
protected hide() {
this._modalShowingContext.get()!.pop();
this._bodyContainer.remove();
this._keydownListener.dispose();
this._resizeListener.dispose();
this.disposableStore.clear();
this._telemetryService.createActionEvent(TelemetryKeys.TelemetryView.Shell, TelemetryKeys.ModalDialogClosed)
.withAdditionalProperties({ name: this._name })
.send();
@@ -416,7 +412,7 @@ export abstract class Modal extends Disposable implements IThemable {
*/
protected addFooterButton(label: string, onSelect: () => void, orientation: 'left' | 'right' = 'right'): Button {
let footerButton = DOM.$('.footer-button');
let button = new Button(footerButton);
let button = this._register(new Button(footerButton));
button.label = label;
button.onDidClick(() => onSelect()); // @todo this should be registered to dispose but that brakes some dialogs
if (orientation === 'left') {
@@ -618,7 +614,6 @@ export abstract class Modal extends Disposable implements IThemable {
public dispose() {
super.dispose();
this._keydownListener.dispose();
this._footerButtons = [];
}
}

View File

@@ -163,7 +163,8 @@ export class OptionsDialog extends Modal {
for (let i = 0; i < options.length; i++) {
let option: azdata.ServiceOption = options[i];
let rowContainer = DialogHelper.appendRow(container, option.displayName, 'optionsDialog-label', 'optionsDialog-input');
OptionsDialogHelper.createOptionElement(option, rowContainer, this._optionValues, this._optionElements, this._contextViewService, (name) => this.onOptionLinkClicked(name));
const optionElement = OptionsDialogHelper.createOptionElement(option, rowContainer, this._optionValues, this._optionElements, this._contextViewService, (name) => this.onOptionLinkClicked(name));
this.disposableStore.add(optionElement.optionWidget);
}
}
@@ -175,12 +176,12 @@ export class OptionsDialog extends Modal {
switch (option.valueType) {
case ServiceOptionType.category:
case ServiceOptionType.boolean:
this._register(styler.attachSelectBoxStyler(<SelectBox>widget, this._themeService));
this.disposableStore.add(styler.attachSelectBoxStyler(<SelectBox>widget, this._themeService));
break;
case ServiceOptionType.string:
case ServiceOptionType.password:
case ServiceOptionType.number:
this._register(styler.attachInputBoxStyler(<InputBox>widget, this._themeService));
this.disposableStore.add(styler.attachInputBoxStyler(<InputBox>widget, this._themeService));
}
}
}
@@ -224,8 +225,8 @@ export class OptionsDialog extends Modal {
}
public close() {
this.dispose();
this.hide();
this._optionElements = {};
this._onCloseEvent.fire();
}
@@ -260,10 +261,6 @@ export class OptionsDialog extends Modal {
public dispose(): void {
super.dispose();
for (let optionName in this._optionElements) {
let widget: Widget = this._optionElements[optionName].optionWidget;
widget.dispose();
delete this._optionElements[optionName];
}
this._optionElements = {};
}
}

View File

@@ -21,7 +21,7 @@ export interface IOptionElement {
}
export function createOptionElement(option: azdata.ServiceOption, rowContainer: HTMLElement, options: { [name: string]: any },
optionsMap: { [optionName: string]: IOptionElement }, contextViewService: IContextViewService, onFocus: (name) => void): void {
optionsMap: { [optionName: string]: IOptionElement }, contextViewService: IContextViewService, onFocus: (name) => void): IOptionElement {
let possibleInputs: string[] = [];
let optionValue = getOptionValueAndCategoryValues(option, options, possibleInputs);
let optionWidget: any;
@@ -63,8 +63,10 @@ export function createOptionElement(option: azdata.ServiceOption, rowContainer:
}
inputElement = findElement(rowContainer, 'input');
}
optionsMap[option.name] = { optionWidget: optionWidget, option: option, optionValue: optionValue };
const optionElement = { optionWidget: optionWidget, option: option, optionValue: optionValue };
optionsMap[option.name] = optionElement;
inputElement.onfocus = () => onFocus(option.name);
return optionElement;
}
export function getOptionValueAndCategoryValues(option: azdata.ServiceOption, options: { [optionName: string]: any }, possibleInputs: string[]): any {