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

monaco preview specification #10136

Merged
merged 8 commits into from
Apr 14, 2021
Merged

monaco preview specification #10136

merged 8 commits into from
Apr 14, 2021

Conversation

Aaron-Junker
Copy link
Collaborator

Summary of the Pull Request

What is this about:

Adds the spec for the monaco preview pane

What is include in the PR:

The spec file

How does someone test / validate:

Read through and check for mistakes

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

@michael-hawker
Copy link
Contributor

We've been experimenting with WebView2 for Monaco in our Monaco Editor repo. It'd be great if we could just support a WPF version of that as well vs. having you re-implement all the work to host Monaco in a WebView?

It's been on my list of things to investigate, I've just been pretty swamped with the Toolkit, but we should be trying to do more with it in April.

We've also been waiting for WebView2 to have better support for AddHostObjectInScript from the WinUI 3 side. I don't know if that works in WPF yet or not?

@Aaron-Junker
Copy link
Collaborator Author

We've been experimenting with WebView2 for Monaco in our Monaco Editor repo. It'd be great if we could just support a WPF version of that as well vs. having you re-implement all the work to host Monaco in a WebView?

It's been on my list of things to investigate, I've just been pretty swamped with the Toolkit, but we should be trying to do more with it in April.

We've also been waiting for WebView2 to have better support for AddHostObjectInScript from the WinUI 3 side. I don't know if that works in WPF yet or not?

It's WPF using a liabary with WebView2

@crutkas
Copy link
Member

crutkas commented Mar 11, 2021

Great job Aaron on this and way to drive clarity. My feedback is all around driving clarity

crutkas added 3 commits March 16, 2021 21:04
added a bit more information up front. Added in new requirement at the bottom that i think we should discuss.
crutkas
crutkas previously approved these changes Mar 17, 2021

### 1.1.2 Why WebView2

Experiments with a POC showed that only WebView2 is displaying Monaco the right way. WebView and WebBrowser elements are using too old versions of IE/Edge who don't support JS functions like `require()`, which are needed for monaco to work. The other option would be to import many JS liabarys, but they don't guarantee 100% coverage with JS and they take more space.
Copy link
Member

Choose a reason for hiding this comment

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

@sibille how does WTS do this without WebView2? It just uses a normal WebView?

Copy link
Member

Choose a reason for hiding this comment

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

my theory is this was added in well before Monaco has this dependencies and why this is like it is

Copy link

Choose a reason for hiding this comment

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

just saw your question: Yes, we added monaco to WTS way before it had this dependency. We're using WPF and WebBrowser.

@crutkas crutkas self-requested a review March 17, 2021 04:24
@crutkas crutkas dismissed their stale review March 17, 2021 04:35

want a q answered :)

@crutkas
Copy link
Member

crutkas commented Mar 18, 2021

I think one item we need to figure out in the POC is what happens when someone doesn't have the runtime installed and safely fall back to a "heyyy, go install the webview". I would say for the installer we'd leverage the bootstrapper installer but that needs to be online.

@crutkas
Copy link
Member

crutkas commented Mar 18, 2021

how we test is on VM and self-contained .net application.

@crutkas
Copy link
Member

crutkas commented Mar 23, 2021

@Aaron-Junker add this bit in about when the webview2 runtime isn't actually installed and lets call it good

@crutkas
Copy link
Member

crutkas commented Mar 30, 2021

@Aaron-Junker any eta? what can i do to help wrap this up

@Aaron-Junker
Copy link
Collaborator Author

@crutkas Do you mean the specification or the project? So the spec can get merged. You're the one who wanted to wait

@htcfreek htcfreek added the Product-File Explorer Power Toys that touch explorer like Preview Pane label Apr 4, 2021
Comment on lines +55 to +64
|Name|Description|Priority|
|----|-----------|--------|
| Working Preview pane | It's simply working. | P0 |
| Settings can install filetypes | When the user activates MonacoPreview in settings it registers the preview handlers. | P0 |
| Style code | Monaco recognizes the file extensions and highlights the syntax the right way. | P0 |
| User can choose file previews | Users can attach custom filetypes to preview. For example `.phptest` files to the `.php` handler. | P2 |
| OOBE | Description for the OOBE. | P1 |
| On/Off in settings | The user can turn it on and off. | P0 |
| Settings: selection for file extensions | User can choose in the settings which File extensions should get registered. | P1 |
| Preview pane Handler registration logic in app, not installer | Logic is migrated out so we deduplicate logic now we're adding 125 files. The uninstaller needs to be run a script of some sort to remove the registration. | P1 |
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Let's order table by priority from P0 as first to P2 as last.
  • Let's move Priority in the first line.
  • Let's add the item "Docs - Adding feature to docs. - P1".

@crutkas crutkas merged commit 2f5458a into master Apr 14, 2021
@crutkas
Copy link
Member

crutkas commented Apr 14, 2021

@crutkas crutkas deleted the users/aaron-junker/monaco-spec branch April 14, 2021 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-File Explorer Power Toys that touch explorer like Preview Pane
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants