Editor preview

[caseyflynn-google]

Fix #1252 Add support for a preview code editor.

[vince-fugnitto]

@caseyflynn-google you are missing the package.spec.ts which can be found in other packages.
This is used for the cli to ensure it passes.

You are also missing the @theia/editor-preview dependency in the package.json found under the example/electron folder.

---

I noticed you introduced a lot of code as part of your PR, but it would also be nice if you included some unit tests as to not to decrease Theia's overall test coverage.

[akosyakov]

@caseyflynn-google cool! really like that optional and not invasive

I've noticed following glitches:

• if I pin one editor and preview another then switching between them by clicking on corresponding files in the navigator does not work
• if I start typing in a preview editor it gets closed and a normal editor gets opened, would it be possible to reuse the preview editor?
• not only a caption of preview editor is in italic style, but an icon as well
• clicking on a file in the navigator activates the preview editor, it would be better if the focus stays on the navigator, otherwise, i never can select a file in the navigator by clicking on it

[akosyakov]

Have you tried just to forward a message to an editor?

MessageLoop.sendMessage(this.editorWidget_, msg);

[caseyflynn-google]

I updated to pass the message, but we will still need to set the size until autoSizing is working. if you like we can move the code to the editor widget.

[akosyakov]

I thought that the editor widget already handles it: https://github.com/theia-ide/theia/blob/e8dd128c25294b23741cc345067952ef7cd85864/packages/editor/src/browser/editor-widget.ts#L75-L81

autoSizing property is not for editors, but for such widgets like REPL, where the editor should be resized if content is changed

[caseyflynn-google]

The way I read / interpreted this is the resize will not be performed here: https://github.com/theia-ide/theia/blob/e8dd128c25294b23741cc345067952ef7cd85864/packages/monaco/src/browser/monaco-editor.ts#L291 due to autoSizing being false.

[akosyakov]

It does not work well in diff editors and when decorations overlapping with folding decorations. - wrong PR

[caseyflynn-google]

@caseyflynn-google cool! really like that optional and not invasive

I've noticed following glitches:

• if I pin one editor and preview another then switching between them by clicking on corresponding files in the navigator does not work

Fixed.

• if I start typing in a preview editor it gets closed and a normal editor gets opened, would it be possible to reuse the preview editor?

I believe this is the current behavior.

• not only a caption of preview editor is in italic style, but an icon as well

Fixed.

• clicking on a file in the navigator activates the preview editor, it would be better if the focus stays on the navigator, otherwise, i never can select a file in the navigator by clicking on it

I am not really sure how to accomplish this, any suggestions would be appreciated.

[caseyflynn-google]

@caseyflynn-google you are missing the package.spec.ts which can be found in other packages.
This is used for the cli to ensure it passes.

You are also missing the @theia/editor-preview dependency in the package.json found under the example/electron folder.

I noticed you introduced a lot of code as part of your PR, but it would also be nice if you included some unit tests as to not to decrease Theia's overall test coverage.

Done.

[caseyflynn-google]

@caseyflynn-google cool! really like that optional and not invasive
I've noticed following glitches:

• if I pin one editor and preview another then switching between them by clicking on corresponding files in the navigator does not work

Fixed.

• if I start typing in a preview editor it gets closed and a normal editor gets opened, would it be possible to reuse the preview editor?

I believe this is the current behavior.

• not only a caption of preview editor is in italic style, but an icon as well

Fixed.

• clicking on a file in the navigator activates the preview editor, it would be better if the focus stays on the navigator, otherwise, i never can select a file in the navigator by clicking on it

I am not really sure how to accomplish this, any suggestions would be appreciated.

Got this fixed.

[akosyakov]

@caseyflynn-google please look at build failures, it seems you forgot to push changes to travis file. It is auto-updated to cache a new extension by travis.

[akosyakov]

@marcdumais-work @svenefftinge Is it possible to add @caseyflynn-google as a Eclipse Theia committer?

[svenefftinge]

@marcdumais-work @svenefftinge Is it possible to add @caseyflynn-google as a Eclipse Theia committer?

Sure, would be great to have you Casey! Are you interested?

[marcdumais-work]

@marcdumais-work @svenefftinge Is it possible to add @caseyflynn-google as a Eclipse Theia committer?

Sure, sounds like a good idea, at least eventually.

Someone would need to nominate him through the Eclipse Foundation tool for that effect, providing a solid case to justify the nomination, to the current committers' satisfaction.

Usually it means listing significant contributions to the project, and making the case of how they warrant the person being made a committer (e.g. some mix of quantity/quality/complexity/rarity of the documented contributions). I have not followed closely enough to know whether that's arguably the case, here, yet.

More details: https://wiki.eclipse.org/Development_Resources/HOWTO/Nominating_and_Electing_a_New_Committer
https://www.eclipse.org/projects/handbook/#elections-nomination

[marcdumais-work]

It would be nice to have a description of what this extension brings. Maybe some bits from the overview part from this comment could be copied: #1252 (comment)

[caseyflynn-google]

#3561

[marcdumais-work]

@caseyflynn-google impressive (first I think?) contribution. Good analysis of the problem in #1252. I just tried the PR on Gitpod, and I think I'll use it daily once merged. I see that beyond the immediately apparent feature, you've added infrastructure to make it easier to support preview for other widgets - even nicer. Thanks!

Just a heads-up: this PR is getting close to the 1000 LoC limit. Over that we will need to submit a CQ, which would delay a bit merging. It looks like it's almost ready to merge, in any case?

[caseyflynn-google]

@caseyflynn-google impressive (first I think?) contribution. Good analysis of the problem in #1252. I just tried the PR on Gitpod, and I think I'll use it daily once merged. I see that beyond the immediately apparent feature, you've added infrastructure to make it easier to support preview for other widgets - even nicer. Thanks!

Glad to hear this is a useful feature!

Just a heads-up: this PR is getting close to the 1000 LoC limit. Over that we will need to submit a CQ, which would delay a bit merging. It looks like it's almost ready to merge, in any case?

I think this is ready to merge. I will add additional documentation in the README.md after this has merged to keep it under the 1000 LoC limit.

[caseyflynn-google]

@marcdumais-work @svenefftinge Is it possible to add @caseyflynn-google as a Eclipse Theia committer?

Sure, would be great to have you Casey! Are you interested?

Absolutely.

[akosyakov]

LGTM

[vince-fugnitto]

@svenefftinge the build still fails due to some leftovers, should we revert?

[marcdumais-work]

Seems related to this commit:

commit 103c95e495f5a0739e70ea6ef14fe2489e1ddfe8
Author: Anton Kosyakov <anton.kosyakov@typefox.io>
Date:   Thu Nov 15 16:24:15 2018 +0000

    [shell] get rid of WidgetTracker
    
    Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

[marcdumais-work]

build error:
[compile] src/browser/frontend-application-module.ts(65,10): error TS2305: Module '"/opt/ericsson-dev/theia/packages/core/src/browser/widgets/index"' has no exported member 'WidgetTracker'.

[marcdumais-work]

Fix: #3555