Skip to content

Commit

Permalink
ui: Don't throw when omnibox.prompt is dimissed
Browse files Browse the repository at this point in the history
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
  • Loading branch information
primiano committed Sep 9, 2024
1 parent 5568871 commit b4a9941
Show file tree
Hide file tree
Showing 6 changed files with 48 additions and 68 deletions.
16 changes: 6 additions & 10 deletions ui/src/core/omnibox_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -25,8 +26,7 @@ export enum OmniboxMode {
interface Prompt {
text: string;
options?: PromptOption[];
resolve(result: string): void;
reject(): void;
resolve(result: Optional<string>): void;
}

const defaultMode = OmniboxMode.Search;
Expand Down Expand Up @@ -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<string> {
prompt(text: string, options?: PromptOption[]): Promise<Optional<string>> {
this._mode = OmniboxMode.Prompt;
this._omniboxSelectionIndex = 0;
this.rejectPendingPrompt();

const promise = new Promise<string>((resolve, reject) => {
const promise = new Promise<Optional<string>>((resolve) => {
this._pendingPrompt = {
text,
options,
resolve,
reject,
};
});

Expand All @@ -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);
}

Expand All @@ -145,7 +141,7 @@ export class OmniboxManagerImpl implements OmniboxManager {

private rejectPendingPrompt() {
if (this._pendingPrompt) {
this._pendingPrompt.reject();
this._pendingPrompt.resolve(undefined);
this._pendingPrompt = undefined;
}
}
Expand Down
40 changes: 18 additions & 22 deletions ui/src/core_plugins/commands/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,35 +296,31 @@ 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);
},
});

ctx.commands.registerCommand({
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);
}
},
});
Expand Down
7 changes: 1 addition & 6 deletions ui/src/core_plugins/debug/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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...');
}
}

Expand Down
27 changes: 12 additions & 15 deletions ui/src/core_plugins/track_utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
});
},
});
}
Expand Down
22 changes: 8 additions & 14 deletions ui/src/frontend/ui_main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
},
},
{
Expand All @@ -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();
},
},
{
Expand Down
4 changes: 3 additions & 1 deletion ui/src/public/omnibox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<string>;
prompt(text: string, options?: PromptOption[]): Promise<Optional<string>>;
}

export interface PromptOption {
Expand Down

0 comments on commit b4a9941

Please sign in to comment.