From b4a99412e67996bc4c4c2fe6952505e43caa12dc Mon Sep 17 00:00:00 2001 From: Primiano Tucci Date: Mon, 9 Sep 2024 13:55:21 +0100 Subject: [PATCH] ui: Don't throw when omnibox.prompt is dimissed The current semantic of the Omnibox API rejects a promise when the prompt is dismissed (Which is the async-equivalent of throwing). This is unideal for various reasons: - Practically: it caused the the codebase to pollute with try/catch blocks that end up suppressing any exception usually in larger scopes, hiding bugs. Virtually nobody wants to deal with exceptions for something that fine grained, and the current state of the codebase reflects that (i.e. everybody ended up over-catching). - This is very error prone: when somebody adds some new code that uses .prompt() they will not realize the subtlety of "if the user presses Esc, this will throw". This behaviour is very undiscoverable, and will lead with a lot of plugins just crashing when the user dimisses the prompt. - Philosophically: exceptions should be used for "exceptional" conditions, typically errors. The user dismissing a prompt is not exceptional. Hence changing the return type into string|undefined, which is more expected. Change-Id: Icee41be84bd2ad0c717b71e3571e0c7f50d8412c --- ui/src/core/omnibox_manager.ts | 16 ++++------ ui/src/core_plugins/commands/index.ts | 40 +++++++++++------------- ui/src/core_plugins/debug/index.ts | 7 +---- ui/src/core_plugins/track_utils/index.ts | 27 +++++++--------- ui/src/frontend/ui_main.ts | 22 +++++-------- ui/src/public/omnibox.ts | 4 ++- 6 files changed, 48 insertions(+), 68 deletions(-) diff --git a/ui/src/core/omnibox_manager.ts b/ui/src/core/omnibox_manager.ts index ea4fef7fe0..df918b7a57 100644 --- a/ui/src/core/omnibox_manager.ts +++ b/ui/src/core/omnibox_manager.ts @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import {Optional} from '../base/utils'; import {OmniboxManager, PromptOption} from '../public/omnibox'; import {raf} from './raf_scheduler'; @@ -25,8 +26,7 @@ export enum OmniboxMode { interface Prompt { text: string; options?: PromptOption[]; - resolve(result: string): void; - reject(): void; + resolve(result: Optional): void; } const defaultMode = OmniboxMode.Search; @@ -97,17 +97,16 @@ export class OmniboxManagerImpl implements OmniboxManager { // Start a prompt. If options are supplied, the user must pick one from the // list, otherwise the input is free-form text. - prompt(text: string, options?: PromptOption[]): Promise { + prompt(text: string, options?: PromptOption[]): Promise> { this._mode = OmniboxMode.Prompt; this._omniboxSelectionIndex = 0; this.rejectPendingPrompt(); - const promise = new Promise((resolve, reject) => { + const promise = new Promise>((resolve) => { this._pendingPrompt = { text, options, resolve, - reject, }; }); @@ -130,10 +129,7 @@ export class OmniboxManagerImpl implements OmniboxManager { // promise to catch, so only do this when things go seriously wrong. // Use |resolvePrompt(null)| to indicate cancellation. rejectPrompt(): void { - if (this._pendingPrompt) { - this._pendingPrompt.reject(); - this._pendingPrompt = undefined; - } + this.rejectPendingPrompt(); this.setMode(OmniboxMode.Search); } @@ -145,7 +141,7 @@ export class OmniboxManagerImpl implements OmniboxManager { private rejectPendingPrompt() { if (this._pendingPrompt) { - this._pendingPrompt.reject(); + this._pendingPrompt.resolve(undefined); this._pendingPrompt = undefined; } } diff --git a/ui/src/core_plugins/commands/index.ts b/ui/src/core_plugins/commands/index.ts index c2e1988ba6..0221484e6a 100644 --- a/ui/src/core_plugins/commands/index.ts +++ b/ui/src/core_plugins/commands/index.ts @@ -296,13 +296,11 @@ class CoreCommandsPlugin implements PerfettoPlugin { id: 'createNewEmptyWorkspace', name: 'Create new empty workspace', callback: async () => { - try { - const name = await ctx.omnibox.prompt('Give it a name...'); - const newWorkspace = new Workspace(name); - globals.workspaces.push(newWorkspace); - globals.switchWorkspace(newWorkspace); - } finally { - } + const name = await ctx.omnibox.prompt('Give it a name...'); + if (name === undefined || name === '') return; + const newWorkspace = new Workspace(name); + globals.workspaces.push(newWorkspace); + globals.switchWorkspace(newWorkspace); }, }); @@ -310,21 +308,19 @@ class CoreCommandsPlugin implements PerfettoPlugin { id: 'switchWorkspace', name: 'Switch workspace', callback: async () => { - try { - const options = globals.workspaces.map((ws) => { - return {key: ws.uuid, displayName: ws.displayName}; - }); - const workspaceUuid = await ctx.omnibox.prompt( - 'Choose a workspace...', - options, - ); - const workspace = globals.workspaces.find( - (ws) => ws.uuid === workspaceUuid, - ); - if (workspace) { - globals.switchWorkspace(workspace); - } - } finally { + const options = globals.workspaces.map((ws) => { + return {key: ws.uuid, displayName: ws.displayName}; + }); + const workspaceUuid = await ctx.omnibox.prompt( + 'Choose a workspace...', + options, + ); + if (workspaceUuid === undefined) return; + const workspace = globals.workspaces.find( + (ws) => ws.uuid === workspaceUuid, + ); + if (workspace) { + globals.switchWorkspace(workspace); } }, }); diff --git a/ui/src/core_plugins/debug/index.ts b/ui/src/core_plugins/debug/index.ts index 50915e5b2c..d17d16078d 100644 --- a/ui/src/core_plugins/debug/index.ts +++ b/ui/src/core_plugins/debug/index.ts @@ -100,12 +100,7 @@ async function getStringFromArgOrPrompt( if (typeof arg === 'string') { return arg; } else { - try { - return await ctx.omnibox.prompt('Enter a query...'); - } catch { - // Prompt was ignored - return undefined; - } + return await ctx.omnibox.prompt('Enter a query...'); } } diff --git a/ui/src/core_plugins/track_utils/index.ts b/ui/src/core_plugins/track_utils/index.ts index 42aec355cd..96e0e647ef 100644 --- a/ui/src/core_plugins/track_utils/index.ts +++ b/ui/src/core_plugins/track_utils/index.ts @@ -56,22 +56,19 @@ class TrackUtilsPlugin implements PerfettoPlugin { return collator.compare(a.displayName, b.displayName); }); - try { - const selectedUri = await ctx.omnibox.prompt( - 'Choose a track...', - sortedOptions, - ); + const selectedUri = await ctx.omnibox.prompt( + 'Choose a track...', + sortedOptions, + ); + if (selectedUri === undefined) return; // Prompt cancelled. - verticalScrollToTrack(selectedUri, true); - const traceTime = globals.traceContext; - globals.selectionManager.setArea({ - start: traceTime.start, - end: traceTime.end, - trackUris: [selectedUri], - }); - } catch { - // Prompt was probably cancelled - do nothing. - } + verticalScrollToTrack(selectedUri, true); + const traceTime = globals.traceContext; + globals.selectionManager.setArea({ + start: traceTime.start, + end: traceTime.end, + trackUris: [selectedUri], + }); }, }); } diff --git a/ui/src/frontend/ui_main.ts b/ui/src/frontend/ui_main.ts index da1fb304a8..fcc4ad498d 100644 --- a/ui/src/frontend/ui_main.ts +++ b/ui/src/frontend/ui_main.ts @@ -110,13 +110,10 @@ export class UiMain implements m.ClassComponent { ]; const promptText = 'Select format...'; - try { - const result = await globals.omnibox.prompt(promptText, options); - setTimestampFormat(result as TimestampFormat); - raf.scheduleFullRedraw(); - } catch { - // Prompt was probably cancelled - do nothing. - } + const result = await globals.omnibox.prompt(promptText, options); + if (result === undefined) return; + setTimestampFormat(result as TimestampFormat); + raf.scheduleFullRedraw(); }, }, { @@ -132,13 +129,10 @@ export class UiMain implements m.ClassComponent { ]; const promptText = 'Select duration precision mode...'; - try { - const result = await globals.omnibox.prompt(promptText, options); - setDurationPrecision(result as DurationPrecision); - raf.scheduleFullRedraw(); - } catch { - // Prompt was probably cancelled - do nothing. - } + const result = await globals.omnibox.prompt(promptText, options); + if (result === undefined) return; + setDurationPrecision(result as DurationPrecision); + raf.scheduleFullRedraw(); }, }, { diff --git a/ui/src/public/omnibox.ts b/ui/src/public/omnibox.ts index bb47948121..6fd6a71415 100644 --- a/ui/src/public/omnibox.ts +++ b/ui/src/public/omnibox.ts @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +import {Optional} from '../base/utils'; + export interface OmniboxManager { /** * Turns the omnibox into an interfactive prompt for the user. Think of @@ -25,7 +27,7 @@ export interface OmniboxManager { * the chosen PromptOption.key if `options` was provided; returns undefined * if the user dimisses the prompt by pressing Esc or clicking eslewhere. */ - prompt(text: string, options?: PromptOption[] | undefined): Promise; + prompt(text: string, options?: PromptOption[]): Promise>; } export interface PromptOption {