-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Rewrite editor-preview package as extensions of editor classes #9518
Rewrite editor-preview package as extensions of editor classes #9518
Conversation
ce92832
to
aacc54e
Compare
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.
This is working much better and it seems to have fixed the issues mentioned above. On a first pass running through the functionality, I only noticed two small behavioral issues related to pinning:
- Splitting the editor view in the main area
- Open 2 files (one being a preview)
- Drag the preview to the right side of the MAIN area to split the screen
- The preview does not pin
- Pinning an editor when it does not exist in the file tree
- Open a file using the steps in Race condition in Editor Preview Manager and Editor Manager #9485 with a file that does not exist in your current workspace
- Observe that the file is opened as a preview widget, but it is not possible to pin it easily (now that the double clicking does not pin)
- In VSCode there is a context command for "Keep Editor Open" which might be a useful approach
aacc54e
to
75c61e8
Compare
@kenneth-marut-work, thanks for taking a look. I've made some changes to address your comments
|
Verified that the new context menu pin option and double clicking work 👍 |
5a11be1
to
7668d2f
Compare
7668d2f
to
d10991a
Compare
packages/editor-preview/src/browser/editor-preview-contribution.ts
Outdated
Show resolved
Hide resolved
684cff7
to
1e9a861
Compare
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 the following functionality 👍
Preference editor.enablePreview
:
- the preference is enabled by default
- when true editors are opened as a preview and replaced
- when false editors are opened normally and not replaced
- when toggling from true to false, the open preview editor is transformed into a normal editor
Application
- a preview editor is transformed into a normal editor when it is modified by a user
- a single-click in the explorer opens a preview editor when the preference is on
- a single-click in the explorer opens a normal editor when the preference is off
- a double-click in the explorer opens a normal editor (preference on)
- renaming a preview editor works correctly - the editor remains a preview
- dragging preview editors works correctly (sidepanel to main-area and so-on)
- keybinding successfully closes a preview editor
- splitting a preview editor works
- reloading the application with preview editors works
- performing toolbar actions (ex: 'markdown: open preview') should work for preview editors
@caseyflynn-google, this may be of interest to you. I plan to merge on Monday or Tuesday, if there are no objections. |
2d6fc03
to
da51685
Compare
@kenneth-marut-work, good find. Since |
58a4adf
to
bf7e2c0
Compare
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.
Awesome work! Verified all items listed in the testing section:
How to test
Basics
- Makes sure that the preference
editor.enablePreview
istrue
and run theRestore workbench layout
command.- Single-click various files in the navigator.
- One editor should be created, and it should be replaced with each new file opened.
- Double click the file you have open in the tree.
- That editor should switch to non-preview mode.
- Single click another file.
- A new editor preview should open up.
- Single click the file in the pinned (non-preview) editor.
- The non-preview editor should be revealed, and no new editor should be opened.
- Focus the current preview editor and start typing.
- It should turn into a normal editor.
Fixes
Renaming
- Open a preview, then rename the file.
- The preview with the old name should be replaced with a new one with the new name.
- Single-clicking a different file should replace the preview.
Navigating
- Open an editor preview in a source code file.
- Use context menu 'Go to references' or Shift + F12 to view a references peek widget
- Clicking on the destinations should work fine.
Dragging
- Open a preview.
- Drag it into a side panel
- You should still see the editor.
- Drag it back
- It should be a non-preview
Rapid opening
- Follow the reproduction instructions in Race condition in Editor Preview Manager and Editor Manager #9485
- You should never wind up with two editors.
Others
- Using ctrl+w (Electron) / alt+w (Browser) to close an editor preview should work.
- Opening a preview editor for e.g. a Markdown file and then using the
Open preview
should work as expected.
Additional checks:
- All context menu items in editor preview tab work (Close, Close Others, Toggle Maximized etc)
- Language server features work in editor preview
- Editor reviews are properly revived (only tested in browser)
d5cd762
to
e70dfb8
Compare
8393b22
to
19049a7
Compare
Signed-off-by: Colin Grant <[email protected]>
19049a7
to
a349c80
Compare
What it does
This PR is a more-or-less wholesale rewrite of the
editor-preview
package to extend and override the functionality of theEditorManager
,EditorWidgetFactory
, andEditorWidget
rather than using a wrapper around anEditorWidget
.These issues aren't directly addressed by these changes, but I can't reproduce them with this code:
Regressions:
double-clicking the title of a preview editor will not pin that editor.Breaking Changes:
Change of various return types inWidgetOpenHandler
and derived classes fromPromise<T | undefined>
toMaybePromise<T> | undefined
to facilitate synchronous checks for widget existence.How to test
Basics
editor.enablePreview
istrue
and run theRestore workbench layout
command.Fixes
Others
Open preview
should work as expected.Review checklist
Reminder for reviewers
Signed-off-by: Colin Grant [email protected]