-
-
Notifications
You must be signed in to change notification settings - Fork 399
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.
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
fix: flawed timing issue when opening editors #1649
Conversation
210513c
to
bc78f4e
Compare
const deferred = new Deferred<EditorWidget>(); | ||
const deferred = new Deferred<EditorWidget>(); | ||
if (!widget) { | ||
// If the editor or is not defined, assume one on create event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this comment. @kittaakos could you please elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking!
We should improve it. I am open to any proposals. Your recent recommendations for clarifying code comments were on the spot 👍
When ensureOpened
is called, the editor can be in two different primary states:
- It's not opened. The editor widget and the backing monaco editor instance do not exist, and the editor widget is not yet registered to the
editorManager
. Thewidget
isundefined
. - It's opened
- the editor widget is the top editor. The editor has the focus (the cursor blinks in it); it's the
activeEditor
. - the editor widget is the top editor, but the focus is on a different widget, or the context menu is active in the editor; it's the
currentEditor
. - the editor widget is not on top. The tab with the filename is visible.
- the editor widget is the top editor. The editor has the focus (the cursor blinks in it); it's the
When the widget
is undefined
, the framework will instantiate it when calling editorManager.open
. After the editor widget creation, editorManager.onCreated
gets an event. IDE2 must handle this case differently, listen to this event, and ensure that the editor is attached to the shell and visible on the screen. Then resolve the promise.
If the widget
is already defined, it's opened; hence, there is no need to wait for any visibility state changes. This case does not need to wait for any events.
Please help to improve the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the comments. Please re-check them. Thank you!
new URI(uri), | ||
options ?? { | ||
mode: 'reveal', | ||
preview: false, | ||
counter: 0, | ||
} | ||
) | ||
.then((editorWidget) => { | ||
// if the widget was already opened we assume it was already already visible and attached to the DOM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// if the widget was already opened we assume it was already already visible and attached to the DOM | |
// if the widget was already opened we assume it was already visible and attached to the DOM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything works as described and I couldn't find any regression with #1563.
Code is also good for me.
From now on, the editor widget open promise resolution does not rely on internal Theia events but solely on @phosphor's `isAttached`/`isVisible` properties. The editor widget promise resolves with the next task after a navigation frame so the browser can render the widget. Closes #1612 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Do not try to parse the original `NotFound` error message, but look for a sketch somewhere in the requested path. Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this fixes the problem of the exceptionally long first load time for multi-file sketches reported at #1612.
I also checked for regressions in handling of opening sketches with invalid format on both Windows and Linux.
Thanks Akos!
Motivation
This PR is to fix the flawed timing issue when opening editors.
Change description
isAttached
/isVisible
properties. The editor widget promise resolves with the next task after a navigation frame so the browser can render the widget.Other information
For the testing, please use the 1612-sketches.zip archive. (However, you're allowed to use your own setup.)
1612-sketches.zip
:Expected behavior:
After opening an invalid sketch, IDE2 prompts a sketch move
The fallback sketch editor is already opened in IDE2 when the modal dialog prompts the move (this is important to preserve the 1.x behavior in IDE2)
The error message is toasted when the IDE2 prompts with the modal dialog.
If one of these 👆 is not working due to a slower or different env OS, please let me know, and I will adjust the timings.
Closes #1612
Reviewer checklist