Skip to content

fix: flawed timing issue when opening editors #1649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 11, 2022
Merged

fix: flawed timing issue when opening editors #1649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc57dec
fix: flawed timing issue when opening editors
Nov 7, 2022
d19ba54
fix: workaround for arduino/arduino-cli#1968
Nov 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 52 additions & 48 deletions arduino-ide-extension/src/browser/contributions/open-sketch-files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { nls } from '@theia/core/lib/common/nls';
import { inject, injectable } from '@theia/core/shared/inversify';
import { injectable } from '@theia/core/shared/inversify';
import type { EditorOpenerOptions } from '@theia/editor/lib/browser/editor-manager';
import { Later } from '../../common/nls';
import { Sketch, SketchesError } from '../../common/protocol';
Expand All @@ -15,14 +15,9 @@ import { ApplicationError } from '@theia/core/lib/common/application-error';
import { Deferred, wait } from '@theia/core/lib/common/promise-util';
import { EditorWidget } from '@theia/editor/lib/browser/editor-widget';
import { DisposableCollection } from '@theia/core/lib/common/disposable';
import { MonacoEditor } from '@theia/monaco/lib/browser/monaco-editor';
import { ContextKeyService as VSCodeContextKeyService } from '@theia/monaco-editor-core/esm/vs/platform/contextkey/browser/contextKeyService';

@injectable()
export class OpenSketchFiles extends SketchContribution {
@inject(VSCodeContextKeyService)
private readonly contextKeyService: VSCodeContextKeyService;

override registerCommands(registry: CommandRegistry): void {
registry.registerCommand(OpenSketchFiles.Commands.OPEN_SKETCH_FILES, {
execute: (uri: URI) => this.openSketchFiles(uri),
Expand Down Expand Up @@ -107,7 +102,7 @@ export class OpenSketchFiles extends SketchContribution {
): Promise<Sketch | undefined> {
const { invalidMainSketchUri } = err.data;
requestAnimationFrame(() => this.messageService.error(err.message));
await wait(10); // let IDE2 toast the error message.
await wait(250); // let IDE2 open the editor and toast the error message, then open the modal dialog
const movedSketch = await promptMoveSketch(invalidMainSketchUri, {
fileService: this.fileService,
sketchService: this.sketchService,
Expand Down Expand Up @@ -135,39 +130,36 @@ export class OpenSketchFiles extends SketchContribution {
const widget = this.editorManager.all.find(
(widget) => widget.editor.uri.toString() === uri
);
if (widget && !forceOpen) {
return widget;
}

const disposables = new DisposableCollection();
if (!widget || forceOpen) {
const deferred = new Deferred<EditorWidget>();
const deferred = new Deferred<EditorWidget>();
// An editor can be in two primary states:
// - The editor is not yet opened. The `widget` is `undefined`. With `editorManager#open`, Theia will create an editor and fire an `editorManager#onCreated` event.
// - The editor is opened. Can be active, current, or open.
// - If the editor has the focus (the cursor blinks in the editor): it's the active editor.
// - If the editor does not have the focus (the focus is on a different widget or the context menu is opened in the editor): it's the current editor.
// - If the editor is not the top editor in the main area, it's opened.
if (!widget) {
// If the widget is `undefined`, IDE2 expects one `onCreate` event. Subscribe to the `onCreated` event
// and resolve the promise with the editor only when the new editor's visibility changes.
disposables.push(
this.editorManager.onCreated((editor) => {
if (editor.editor.uri.toString() === uri) {
if (editor.isVisible) {
disposables.dispose();
if (editor.isAttached && editor.isVisible) {
deferred.resolve(editor);
} else {
// In Theia, the promise resolves after opening the editor, but the editor is neither attached to the DOM, nor visible.
// This is a hack to first get an event from monaco after the widget update request, then IDE2 waits for the next monaco context key event.
// Here, the monaco context key event is not used, but this is the first event after the editor is visible in the UI.
disposables.push(
(editor.editor as MonacoEditor).onDidResize((dimension) => {
if (dimension) {
const isKeyOwner = (
arg: unknown
): arg is { key: string } => {
if (typeof arg === 'object') {
const object = arg as Record<string, unknown>;
return typeof object['key'] === 'string';
}
return false;
};
disposables.push(
this.contextKeyService.onDidChangeContext((e) => {
// `commentIsEmpty` is the first context key change event received from monaco after the editor is for real visible in the UI.
if (isKeyOwner(e) && e.key === 'commentIsEmpty') {
deferred.resolve(editor);
disposables.dispose();
}
})
editor.onDidChangeVisibility((visible) => {
if (visible) {
// wait an animation frame. although the visible and attached props are true the editor is not there.
// let the browser render the widget
setTimeout(
() =>
requestAnimationFrame(() => deferred.resolve(editor)),
0
);
}
})
Expand All @@ -176,29 +168,41 @@ export class OpenSketchFiles extends SketchContribution {
}
})
);
this.editorManager.open(
}

this.editorManager
.open(
new URI(uri),
options ?? {
mode: 'reveal',
preview: false,
counter: 0,
}
)
.then((editorWidget) => {
// If the widget was defined, it was already opened.
// The editor is expected to be attached to the shell and visible in the UI.
// The deferred promise does not have to wait for the `editorManager#onCreated` event.
// It can resolve earlier.
if (!widget) {
deferred.resolve(editorWidget);
}
});

const timeout = 5_000; // number of ms IDE2 waits for the editor to show up in the UI
const result = await Promise.race([
deferred.promise,
wait(timeout).then(() => {
disposables.dispose();
return 'timeout';
}),
]);
if (result === 'timeout') {
console.warn(
`Timeout after ${timeout} millis. The editor has not shown up in time. URI: ${uri}`
);
const timeout = 5_000; // number of ms IDE2 waits for the editor to show up in the UI
const result = await Promise.race([
deferred.promise,
wait(timeout).then(() => {
disposables.dispose();
return 'timeout';
}),
]);
if (result === 'timeout') {
console.warn(
`Timeout after ${timeout} millis. The editor has not shown up in time. URI: ${uri}`
);
}
return result;
}
return result;
}
}
export namespace OpenSketchFiles {
Expand Down
58 changes: 11 additions & 47 deletions arduino-ide-extension/src/electron-main/theia/electron-main-application.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { fork } from 'child_process';
import { AddressInfo } from 'net';
import { join, isAbsolute, resolve } from 'path';
import { promises as fs, Stats } from 'fs';
import { promises as fs } from 'fs';
import { MaybePromise } from '@theia/core/lib/common/types';
import { ElectronSecurityToken } from '@theia/core/lib/electron-common/electron-token';
import { FrontendApplicationConfig } from '@theia/application-package/lib/application-props';
Expand All @@ -28,6 +28,7 @@ import {
SHOW_PLOTTER_WINDOW,
} from '../../common/ipc-communication';
import { ErrnoException } from '../../node/utils/errors';
import { isAccessibleSketchPath } from '../../node/sketches-service-impl';

app.commandLine.appendSwitch('disable-http-cache');

Expand Down Expand Up @@ -145,7 +146,10 @@ export class ElectronMainApplication extends TheiaElectronMainApplication {
event.preventDefault();
const resolvedPath = await this.resolvePath(path, cwd);
if (resolvedPath) {
const sketchFolderPath = await this.isValidSketchPath(resolvedPath);
const sketchFolderPath = await isAccessibleSketchPath(
resolvedPath,
true
);
if (sketchFolderPath) {
this.openFilePromise.reject(new InterruptWorkspaceRestoreError());
await this.openSketch(sketchFolderPath);
Expand All @@ -158,49 +162,6 @@ export class ElectronMainApplication extends TheiaElectronMainApplication {
}
}

/**
* The `path` argument is valid, if accessible and either pointing to a `.ino` file,
* or it's a directory, and one of the files in the directory is an `.ino` file.
*
* If `undefined`, `path` was pointing to neither an accessible sketch file nor a sketch folder.
*
* The sketch folder name and sketch file name can be different. This method is not sketch folder name compliant.
* The `path` must be an absolute, resolved path.
*/
private async isValidSketchPath(path: string): Promise<string | undefined> {
let stats: Stats | undefined = undefined;
try {
stats = await fs.stat(path);
} catch (err) {
if (ErrnoException.isENOENT(err)) {
return undefined;
}
throw err;
}
if (!stats) {
return undefined;
}
if (stats.isFile()) {
return path.endsWith('.ino') ? path : undefined;
}
try {
const entries = await fs.readdir(path, { withFileTypes: true });
const sketchFilename = entries
.filter((entry) => entry.isFile() && entry.name.endsWith('.ino'))
.map(({ name }) => name)
.sort((left, right) => left.localeCompare(right))[0];
if (sketchFilename) {
return join(path, sketchFilename);
}
// If no sketches found in the folder, but the folder exists,
// return with the path of the empty folder and let IDE2's frontend
// figure out the workspace root.
return path;
} catch (err) {
throw err;
}
}

private async resolvePath(
maybePath: string,
cwd: string
Expand Down Expand Up @@ -253,7 +214,10 @@ export class ElectronMainApplication extends TheiaElectronMainApplication {
if (!resolvedPath) {
continue;
}
const sketchFolderPath = await this.isValidSketchPath(resolvedPath);
const sketchFolderPath = await isAccessibleSketchPath(
resolvedPath,
true
);
if (sketchFolderPath) {
workspace.file = sketchFolderPath;
if (this.isTempSketch.is(workspace.file)) {
Expand Down Expand Up @@ -284,7 +248,7 @@ export class ElectronMainApplication extends TheiaElectronMainApplication {
if (!resolvedPath) {
continue;
}
const sketchFolderPath = await this.isValidSketchPath(resolvedPath);
const sketchFolderPath = await isAccessibleSketchPath(resolvedPath, true);
if (sketchFolderPath) {
path = sketchFolderPath;
break;
Expand Down
100 changes: 53 additions & 47 deletions arduino-ide-extension/src/node/sketches-service-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,62 +734,68 @@ function isNotFoundError(err: unknown): err is ServiceError {

/**
* Tries to detect whether the error was caused by an invalid main sketch file name.
* IDE2 should handle gracefully when there is an invalid sketch folder name. See the [spec](https://arduino.github.io/arduino-cli/latest/sketch-specification/#sketch-root-folder) for details.
* The CLI does not have error codes (https://github.com/arduino/arduino-cli/issues/1762), so IDE2 parses the error message and tries to guess it.
* IDE2 should handle gracefully when there is an invalid sketch folder name.
* See the [spec](https://arduino.github.io/arduino-cli/latest/sketch-specification/#sketch-root-folder) for details.
* The CLI does not have error codes (https://github.com/arduino/arduino-cli/issues/1762),
* IDE2 cannot parse the error message (https://github.com/arduino/arduino-cli/issues/1968#issuecomment-1306936142)
* so it checks if a sketch even if it's invalid can be discovered from the requested path.
* Nothing guarantees that the invalid existing main sketch file still exits by the time client performs the sketch move.
*/
async function isInvalidSketchNameError(
cliErr: unknown,
requestSketchPath: string
): Promise<string | undefined> {
if (isNotFoundError(cliErr)) {
const ino = requestSketchPath.endsWith('.ino');
if (ino) {
const sketchFolderPath = path.dirname(requestSketchPath);
const sketchName = path.basename(sketchFolderPath);
const pattern = escapeRegExpCharacters(
`${invalidSketchNameErrorRegExpPrefix}${path.join(
sketchFolderPath,
`${sketchName}.ino`
)}`
);
if (new RegExp(pattern, 'i').test(cliErr.details)) {
try {
await fs.access(requestSketchPath);
return requestSketchPath;
} catch {
return undefined;
}
}
} else {
try {
const resources = await fs.readdir(requestSketchPath, {
withFileTypes: true,
});
return (
resources
.filter((resource) => resource.isFile())
.filter((resource) => resource.name.endsWith('.ino'))
// A folder might contain multiple sketches. It's OK to ick the first one as IDE2 cannot do much,
// but ensure a deterministic behavior as `readdir(3)` does not guarantee an order. Sort them.
.sort(({ name: left }, { name: right }) =>
left.localeCompare(right)
)
.map(({ name }) => name)
.map((name) => path.join(requestSketchPath, name))[0]
);
} catch (err) {
if (ErrnoException.isENOENT(err) || ErrnoException.isENOTDIR(err)) {
return undefined;
}
throw err;
}
return isNotFoundError(cliErr)
? isAccessibleSketchPath(requestSketchPath)
: undefined;
}

/**
* The `path` argument is valid, if accessible and either pointing to a `.ino` file,
* or it's a directory, and one of the files in the directory is an `.ino` file.
*
* `undefined` if `path` was pointing to neither an accessible sketch file nor a sketch folder.
*
* The sketch folder name and sketch file name can be different. This method is not sketch folder name compliant.
* The `path` must be an absolute, resolved path. This method does not handle EACCES (Permission denied) errors.
*
* When `fallbackToInvalidFolderPath` is `true`, and the `path` is an accessible folder without any sketch files,
* this method returns with the `path` argument instead of `undefined`.
*/
export async function isAccessibleSketchPath(
path: string,
fallbackToInvalidFolderPath = false
): Promise<string | undefined> {
let stats: Stats | undefined = undefined;
try {
stats = await fs.stat(path);
} catch (err) {
if (ErrnoException.isENOENT(err)) {
return undefined;
}
throw err;
}
if (!stats) {
return undefined;
}
if (stats.isFile()) {
return path.endsWith('.ino') ? path : undefined;
}
const entries = await fs.readdir(path, { withFileTypes: true });
const sketchFilename = entries
.filter((entry) => entry.isFile() && entry.name.endsWith('.ino'))
.map(({ name }) => name)
// A folder might contain multiple sketches. It's OK to pick the first one as IDE2 cannot do much,
// but ensure a deterministic behavior as `readdir(3)` does not guarantee an order. Sort them.
.sort((left, right) => left.localeCompare(right))[0];
if (sketchFilename) {
return join(path, sketchFilename);
}
return undefined;
// If no sketches found in the folder, but the folder exists,
// return with the path of the empty folder and let IDE2's frontend
// figure out the workspace root.
return fallbackToInvalidFolderPath ? path : undefined;
}
const invalidSketchNameErrorRegExpPrefix =
'.*: main file missing from sketch: ';

/*
* When a new sketch is created, add a suffix to distinguish it
Expand Down