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

Support Preview Editors #1252

Closed
svenefftinge opened this issue Feb 12, 2018 · 12 comments
Closed

Support Preview Editors #1252

svenefftinge opened this issue Feb 12, 2018 · 12 comments

Comments

@svenefftinge
Copy link
Contributor

Currently, Theia always opens a new editor widget, when

  • double clicking the editor
  • use open file command
  • double-clicking an entry in a git view

When going through a list of changes in the git view, this leaves the user with many open editors. In VSCode there is the notion of a preview editor that is reused. The logic is that a single click will reuse an existing preview editor or open one if none is open yet. A double click or a file change will turn a preview editor into a persistent one.

While I found this behavior a bit unintuitiv, I don't know a better way to allow the user control this and do the right thing by default. Also given that Theia should be a good online experience for people using VSCode locally, I propose to simply copy this behavior.

@elaihau elaihau self-assigned this Apr 5, 2018
@elaihau elaihau removed their assignment Jun 21, 2018
@elaihau
Copy link
Contributor

elaihau commented Jun 21, 2018

@vince-fugnitto is working on this one

@paul-marechal
Copy link
Member

The QuickOpenItems also support preview events, so that when you highlight an item you can trigger the preview mode, this is what is done on vscode, and it would play nice with the preview-editor feature.

@caseyflynn-google
Copy link
Contributor

Is work still in progress for this request?

@elaihau
Copy link
Contributor

elaihau commented Sep 26, 2018

@caseyflynn-google
No. @vince-fugnitto worked on this one and got other priorities. Feel free to take this one if you want.

@caseyflynn-google
Copy link
Contributor

@elaihau Sounds good, I will go ahead and start looking into this. @svenefftinge Do you have any suggestions / guidance on how this should be implemented?

@elaihau elaihau assigned elaihau and unassigned elaihau Oct 3, 2018
@svenefftinge
Copy link
Contributor Author

It's a rather complicated thing to add, at least if you want to reuse editors. In renaming we close and open the editor as that is more robust. But it is not ideal either.
OTOH even after using VS Code for almost 3 years now, I find the behavior of pinning an editor confusing.
But if it's an important feature for you, please go ahead and look into. We will try to help you.
I think a good start would be fully to understand the behavior in VS Code. I.e. when is an editor opened in pinned mode and when not.
In Theia it would be the responsibility of the editor manager to accept an option for that, and based on it 'reuse' an existing or open a new editor. @akosyakov Any further tips?

@caseyflynn-google
Copy link
Contributor

caseyflynn-google commented Oct 9, 2018

I have been researching this a bit and it looks like the biggest barrier lies with the widget-manager as it maintains a 1-1 mapping between a widget and its options (specifically, the options include a file URI when dealing with an editor). If we proceed with modifying the editor manager I think we would have to close and open editors each time we wish to preview a file (due to the mapping in the widget-manager) which is not ideal.
We could instead create a preview editor and associated provider. This would negate the need to close / reopen the widget each time a file is previewed, but we would still have to close / reopen when transitioning to a permanent editor (not ideal either). Going this route would also require a 1-1 mapping of widget to preview-enabled widget (not very sustainable).
Lastly, we could refactor WidgetManager to enable it to be provided by another extension. We could then create a widget manager that has the ability to manage it’s own “preview widget” which would merely wrap an underlying widget and attach listeners to determine when to transition the preview to a permanent widget. Thoughts?

@caseyflynn-google
Copy link
Contributor

caseyflynn-google commented Oct 10, 2018

Theia Preview Tab Support

Status: Draft
Last Updated: 2018-10-10

Objective

Enable support for Preview Editors in Theia.

Overview

A preview editor supports the same functionality as a regular editor widget with the exception: if a the preview editor has not "transitioned to a permanent editor" at the time an additional request to preview a file is received, instead of opening a new editor, it will display the contents of the newly requested
file. Events that will transition the editor to a permanent editor are defined as:

  1. Modifying the file in the Preview Editor.
  2. Double clicking the file that is being previewed in the file navigator
  3. Double clicking the preview editor tab
  4. Performing a drag/drop operation on the preview editor tab that results in
    the tab index of the preview editor being changed, or being transitioned to
    another area within the application.

Currently all widget management in Theia is performed by the WidgetManager (provided as a singleton instance via inversify). WidgetManager maintains a collection of widgets keyed by the the factory id which created the widget and associated options passed to the factory. This enables the manager to perform a constant time lookup when a request is made to retrieve (open or create) a given widget, while a the same time introducing a 1:1 mapping between factory+options and a given widget.

The current implementation of the EditorWidget requires a file URI be provided in the widget to associate the content that will be managed in the widget with content on disk. Thus there is a 1:1 mapping between a given file and editor widget. Taking into account the constraint introduced by the WidgetManager, currently there can be only one Editor Widget associated associated with any
given file.

Design Options

Modify Editor Manager (Editor Widget Factory) to manage editor widgets, manage an ephemeral editor widget for preview

In this scenario we would modify EditorManager to manage an ephemeral editor widget and add logic to the EditorManager track all editor widgets (either via calls to WidgetManager, or maintain its own collection of widgets). When a call is made to preview a file, the editor manager would first check all open editor widget it is managing, if a widget displaying the file is found, the given editor widget is selected. If there is no editor associated with the requested file, it would create or recreate (that is, close the existing then create) the ephemeral editor widget with the requested "preview" file. The editor manager would be responsible for observing and reacting to events which would cause the ephemeral editor widget to become a "permanent" editor widget (updating the status of the widget and modifying UI elements).

Pros:
Relatively small and self contained change

Cons:
Undesirable creation / destruction of widgets in "preview mode"
Only allows preview of Editor Widgets

Create a separate Preview Widget and associated Factory

In this scenario we would add an Editor Preview Widget factory responsible for managing a specialized editor widget that does not associate the widget with the file it is currently previewing. The factory would have the ability to update the content of the preview widget when a request to open a new preview widget is received. It would also be responsible for determining if the request to preview a file already has a permanent editor view associated with the given file and activating the widget if it already exists (similar to the option above). This approach, however, lacks the ability to transition the Editor Preview Widget to a (regular) Editor Widget, as it cannot modify its mapping created in WidgetManager (the factory ids will be different). Thus, to do the transition, the Preview Widget would be destroyed and an Editor Widget would be created.

Pros:
Relatively small and self contained change
Can be deployed as a package as opposed to modifying the existing editor package

Cons:
Still requires the destruction of the preview widget when the editor is marked as a permanent editor
Only allows preview of Editor Widgets

Modify Editor Manager to manage editor widgets and maintain a preview widget

This can be likened to a combination of the first two options; we would augment EditorManager to perform the management of Editor Widgets. We would then create a subclass of editor widget (or augment the current editor widget) to support the ability to arbitrarily change the underlying text editor and retrieve the currently displayed file URI. The editor creation options for the preview editor would include a unique id as opposed to a file URI to disassociate the notion of the preview editor representing a single file URI. As with the first option, the editor manager would likewise be responsible for transitioning the Preview Widget to a permanent Editor Widget.

Pros:
Removes the need to destroy / recreate widgets

Cons:
Slightly more complex implementation
Only allows preview of Editor Widgets

Allow Widget Manager to be Injected, override it in a new package

In this scenario we would modify the Core package to allow for the injection of a WidgetManager by abstracting an interface, then bind the current implementation in the front-end-application module. After this is complete, we could create a package that contains an alternate WidgetManager that maintains its own notion of a Preview Widget and manages widgets accordingly.

Pros:
No need to destroy / recreate widgets
Allows preview of any arbitrary widget

Cons:
More complex development effort
Would need to define a standard API for monitoring when arbitrary widgets wish to transition to permanent widgets

@akosyakov
Copy link
Member

akosyakov commented Oct 16, 2018

@caseyflynn-google thank you for the thorough analysis.

It would be nice if we avoid changing existing APIs and contracts of the editor and widget managers. I would imagine that from the client perspective the only difference is passing an additional option, i.e.

editorManager.open(uri, { preview: true });

I tend to the approach with the editor preview widget which is API compatible with the editor widget and implemented via composition. Such preview widget can have its own factory and based on ids instead of URIs. The editor manager depending on given preview option and pinned state of existing preview widgets will decide whether a new preview widget should be created or an editor reference of an existing should be updated, i.e. if the preview editor widget gets pinned it means that its editor reference cannot be updated anymore, but cliens can continue using it as an editor widget. TrackableWidgetProvider should be implemented by such widget to make the shell aware of editors.

Such approach does not need changes for any existing contracts and does not have any UI side-effects. What do you think?

Only allows preview of Editor Widgets

For other widgets we already have the preview extension, so it should be fine.

@paul-marechal
Copy link
Member

paul-marechal commented Oct 16, 2018

For other widgets we already have the preview extension, so it should be fine.

The preview extension serves a different purpose if I understand correctly, which is opening a new widget to display some render of a source file.

Here "preview" is intended as opening some widget in a shared spot, so to speak:

vscode-preview-editors

@caseyflynn-google
Copy link
Contributor

caseyflynn-google commented Oct 16, 2018

Such preview widget can have its own factory and based on ids instead of URIs.

To clarify, are you recommending we override the open logic in EditorManager to allow different factory ids to be provided to the base WidgetOpenHandler? Or are you recommending we implement additional logic to call the widget manager directly as opposed to using WidgetOpenHandler?

The editor manager depending on given preview option and pinned state of existing preview widgets will decide whether a new preview widget should be created or an editor reference of an existing should be updated

This is a minor implementation detail, but is a fairly important distinction as it puts the onus of tracking all editor widgets in the Editor Manager as Opposed to the Widget Manager. (Requests to preview an open file should return the open editor which could be an editor widget or a preview widget. Requests to open a file being previewed should "pin" an existing preview editor if it has the file open).

Only allows preview of Editor Widgets

For other widgets we already have the preview extension, so it should be fine.

The preview extension serves a different purpose if I understand correctly, which is opening a new widget to display some render of a source file.

As pointed out by @marechal-p The preview extension does not cover this use case. I was pointing out that we would not have a way to preview any other type of widget. E.G. if there is a widget that allows for composition of uml vi a wysiwyg editor, we would be unable to preview it (or in the case of the preview widget, we cannot preview the preview widget).

@akosyakov @svenefftinge Are you OK with these details?

@caseyflynn-google
Copy link
Contributor

I went ahead and implemented this as a separate extension as opposed to modifying the existing editor manager. Please let me know what you think 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants