Skip to content
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

Allow EditorManager to Open with Selection before Attachment #9004

Conversation

colin-grant-work
Copy link
Contributor

@colin-grant-work colin-grant-work commented Jan 30, 2021

Signed-off-by: Colin Grant [email protected]

What it does

This PR fixes #8955 by interposing EditorManager.revealSelection between the creation of an editor widget and its attachment / opening. Previously, an editor would be attached, either independently or as part of an EditorPreviewWidget, and then revealSelection would be called. As a consequence, EditorManager.onCurrentEditorChangedEmitter was fired when the editor was picked up by the shell, before revealSelection had been called, and the top of the file would always be added to the navigation stack. With this PR, revealSelection can be called before the widget is picked up by the shell.

Alternative solutions to the problem include:

  1. Rigging an event to fire when revealSelection is run and making the navigation system respond by removing the top of the relevant file from the top of the navigation stack. (Works pretty well)
  2. Preventing the EditorManager from firing the onCurrentEditorChanged event for a widget that hasn't been opened yet. (Haven't tried, EditorPreviews - the most problematic case - do eventually call open.)
  3. Any others you think would be cleaner than this modification of the get(OrCreate)ByUri mechanism.

How to test

  1. Open a workspace with files in a language that provides definitions (e.g. TS files in the Theia repository)
  2. Find reference to something defined in another file.
  3. ctrl/cmd+click through to the definition.
  4. Activate the command palette and select Go back
  5. Observe that you navigate back the first file, without an intermediate stop at the start of the second file.

Review checklist

Reminder for reviewers

@colin-grant-work colin-grant-work force-pushed the bugfix/nav-stack-on-open branch 2 times, most recently from 3b4e2fd to baaf722 Compare January 30, 2021 04:32
@colin-grant-work colin-grant-work added editor issues related to the editor editor-preview issues that are related to the editor-preview labels Jan 30, 2021
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some syntax errors (missing semicolon) which failed the build.
We may want to rebase the pull-request on master to resolve the issue with downloading the builtin extensions.

packages/editor/src/browser/editor-manager.ts Outdated Show resolved Hide resolved
packages/editor/src/browser/editor-manager.ts Outdated Show resolved Hide resolved
@colin-grant-work colin-grant-work force-pushed the bugfix/nav-stack-on-open branch from baaf722 to 20a0a3a Compare February 3, 2021 15:22
@vince-fugnitto
Copy link
Member

@colin-grant-work looks like there is two different Colin authors :)

@colin-grant-work
Copy link
Contributor Author

@colin-grant-work looks like there is two different Colin authors :)

I keep trying to expunge the other one from the git history, but he's quite persistent! I'll make another attempt...

@colin-grant-work colin-grant-work force-pushed the bugfix/nav-stack-on-open branch from 20a0a3a to 1622c24 Compare February 3, 2021 15:42
@colin-grant-work
Copy link
Contributor Author

colin-grant-work commented Feb 3, 2021

I believe the ghost has been banished, and the code seems to be up to date, despite a slow pipeline between my fork and this PR.

@colin-grant-work colin-grant-work force-pushed the bugfix/nav-stack-on-open branch 2 times, most recently from a0afbdc to 6060fe7 Compare February 3, 2021 16:06
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@colin-grant-work the changes look good to me, and they fix the described issue 👍

This commit ensures that if a selection is passed to the editor-manager
when an editor is created, the selection is revealed before the widget
is attached.

Signed-off-by: Colin Grant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor editor-preview issues that are related to the editor-preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking through function takes you to top of file first
2 participants