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

Code inset feature #66418

Merged
merged 14 commits into from Feb 13, 2019
Merged

Code inset feature #66418

merged 14 commits into from Feb 13, 2019

Conversation

rdeline
Copy link
Contributor

@rdeline rdeline commented Jan 11, 2019

This PR introduces a new feature to allow an HTML document to be inset inside a code editor at a given line of code. This feature is exposed to extensions to support a variety of use cases. To illustrate the feature, the PR includes an extension that looks for specially formatted comments in JS/TS files, e.g.
// INLINE some-url
and embeds the contents of that URL directly below the comment line. Another use case is to support a user experience like computations notebooks (e.g. Jupyter) in which script results are shown inlined inside the script code.
This PR is an MVP to illustrate the feature.

@RMacfarlane
Copy link
Contributor

Hi @rdeline, is there an open issue tracking this feature request? Please see our contributing guidelines, we ask that PRs address open issues.

Things like new extension APIs must be discussed with and agreed upon by the feature owner

@rdeline
Copy link
Contributor Author

rdeline commented Jan 12, 2019

Sorry, @RMacfarlane, it's my first PR, so I don't know the conventions. :) Relevant issues include 41775 (which this PR directly addresses), and 34739, and 39492 (which this PR enables, with a further change in the Python extension, which I've also done).

@jrieken
Copy link
Member

jrieken commented Jan 18, 2019

Linking #3220 which I believe was the first request of this kind

@leocb
Copy link

leocb commented Jan 23, 2019

Would this PR allow the HTML/JS to "return" something to the code? example: #66493

@rdeline
Copy link
Contributor Author

rdeline commented Jan 23, 2019

@leocb Yes, it could. The feature is implemented using a webview element, and there is a communication channel between Code and the webview. So, the JS code hosted in the webview can post a message, which the extension can listen for. In the PR, the inline-doc extension uses this approach to communicate the size of the HTML content.

@tecosaur
Copy link

tecosaur commented Jan 27, 2019

@jrieken Looking at the VS Code PR requests, I see some open from 2016, and some merged two days ago. As a result, I thought it was worth asking - is it, and if so when is it, likely that this PR will be merged?

( I come from an extension that is deeply interested in this functionality)

@jrieken
Copy link
Member

jrieken commented Jan 28, 2019

@tecosaur Yes - we do have interest in the PR. No - we don't have a timeline.

@jrieken
Copy link
Member

jrieken commented Jan 28, 2019

@rdeline Thanks so far. I did take this for spin and things look promising. Everything is weird into the right places and it's more about the following items

  • test how well this works in real life as every web view is a separate process.
  • tweak the API

Wrt the API we actually wanna go away from the URL-approach and expose the WebViews instead. Something like this

class CodeInset {
  id: string;
  range: Range;
  height?:number;
}

export interface CodeInsetProvider {
  //...

  resolveCodeInset(inset: CodeInset, webview: Webview, token: CancellationToken): Promise<void>
}

So, the extension provides the insets and the editor calls back with a web view which can be managed by the extension, e.g. webview.html = someHtml. That will also enable the bidi-communication it supports and webview-code will know when a web view is being terminated etc.

@jrieken jrieken added feature-request Request for new features or functionality api api-proposal labels Jan 28, 2019
@rdeline
Copy link
Contributor Author

rdeline commented Feb 2, 2019

@jrieken I like this tweak to the API, particularly since it means that extensions won't have to implement both a CodeInsetProvider and TextDocumentContentProvider to put original HTML in the editor. This suggestion nicely combines these into one class.

I'm trying to update the PR to implement this change, but it's a struggle. The issue is this. The webview needs to be created by MainThreadWebviews.$createWebview in order for an extension to make calls on it. However, the webview also needs to be available to the code inset editor contribution, namely CodeInsetController. So far, I'm not seeing a good way to wire these two components together. Any suggestions would be very welcome. Maybe MainThreadWebviews needs to be a service so that the code inset editor contribution can ask for it in its constructor.

@jrieken jrieken added this to the February 2019 milestone Feb 7, 2019
@rdeline
Copy link
Contributor Author

rdeline commented Feb 7, 2019

I've updated the PR to reflect the requested API change that @jrieken requested. The new API is definitely nicer as can be seen in extensions/inline-doc/extension.ts. I found a way to wire up the pieces that I think is reasonable, but of course all suggestions are welcome.

I think there's an interesting speed/space trade-off with this feature. Webviews both take up memory and are noticeably slow to load. To optimize for space, one could allocate webviews only for the currently visible viewport, but the loading delay would be noticeable to the user. I think it's up to the core Code team to decide an appropriate policy for this trade-off.

@jrieken
Copy link
Member

jrieken commented Feb 11, 2019

@rdeline We have finished our February planning and we wanna go forward with this. The goal is to get this merged quickly and that we take over for the final bits. No promise on when we expose this a real API, for now this will be proposed and we will experiment with the overall performance first. As next steps I would propose

  • merge with master one more time
  • move the sample extension so that it doesn't become part of vs code

@kieferrm kieferrm mentioned this pull request Feb 11, 2019
35 tasks
@gulshan
Copy link

gulshan commented Feb 12, 2019

Can we get a screenshot please?

@jrieken
Copy link
Member

jrieken commented Feb 12, 2019

@rdeline I have squashed and rebased your commits into 98aec46. I have also pushed a few commits which makes code insets a workbench feature - I moved declarations of interfaces and settings. Also, I have pushed 2517974 which undoes some seemingly unrelated changes.

@rdeline
Copy link
Contributor Author

rdeline commented Feb 12, 2019

Screen shot of the sample extension to inline arbitrary URLs inside the code:

screen shot 2019-01-11 at 2 56 11 pm

@samghelms
Copy link

samghelms commented Apr 4, 2019

@rdeline @jrieken Do you have any examples on how to get this api up and running? I see that there was an example and it looks like it got force push overwritten. It's unclear how to actually trigger the extension after it is registered.

@jrieken
Copy link
Member

jrieken commented Jun 4, 2019

fyi - while tackling some debt I have also jumped on this api proposal and I have changed it to a push model. surely some new regressions have been added, but the API really looks nice:

// #region Joh - code insets
export interface WebviewEditorInset {
readonly editor: TextEditor;
readonly range: Range;
readonly webview: Webview;
readonly onDidDispose: Event<void>;
dispose(): void;
}
export namespace window {
export function createWebviewTextEditorInset(editor: TextEditor, range: Range, options?: WebviewOptions): WebviewEditorInset;
}
//#endregion

@jrieken
Copy link
Member

jrieken commented Jun 4, 2019

sample, implements command that adds an inset:

    vscode.commands.registerCommand('extension.sayHello', async (args, brgs, crgs) => {
        if (!vscode.window.activeTextEditor) {
            return;
        }

        const inset = vscode.window.createWebviewTextEditorInset(
            vscode.window.activeTextEditor,
            vscode.window.activeTextEditor.selection.with({ end: vscode.window.activeTextEditor.selection.end.translate(8) }),
            // { enableScripts: true, enableCommandUris: true }
        );
        inset.onDidDispose(() => {
            console.log('WEBVIEW disposed...');
        });
        inset.webview.html = `<head><meta></head><body><img src="https://imgs.xkcd.com/comics/plutonium.png"/><body>`;
    });

@samghelms
Copy link

@jrieken Is there a way to make the webview html interactive? (So that the user can click on a button in the code inset, for example)

@tamuratak
Copy link
Contributor

@jrieken Great work. The most astonishing point is that this is so fast and lightweight.

I have a question. Insets disappear when we change tabs. Is this an intended behavior?

Jun-08-2019 16-33-58

I am looking forward to this feature coming to the stable branch.

@rdeline
Copy link
Contributor Author

rdeline commented Jun 11, 2019

@jrieken I love the simplified the API, but I'm wondering about this code in the implementation:

		const webview = this._webviewService.createWebview({
			enableFindWidget: false,
			allowSvgs: false,
			extension: undefined
		}, {
				allowScripts: options.enableScripts
			});

As far as I can tell, the fact that extension is null means that the vscode-resource protocol will not be defined properly for the webview, since extension location will be undefined. This means that extensions can't load local resources into inset webviews, which is a real limitation. I think the typical use case is that an extensions will ship with some HTML to be displayed. This is what I'm trying to do in the Python extension, for example.

@jrieken
Copy link
Member

jrieken commented Jun 12, 2019

Insets disappear when we change tabs. Is this an intended behavior

Yes - for the API it means the editor goes aways and comes back (as new editor) and therefore you need to re-add them. This is inline with how decorations work.

As far as I can tell, the fact that extension is null means that the vscode-resource protocol will not be defined properly for the webview,

Ops, that was an oversight and not in the intent. I will push a fix for that

@rdeline
Copy link
Contributor Author

rdeline commented Jun 17, 2019

@jrieken Another piece of feedback on the new API, based on my PR for the Python extension to inline Jupyter results. First, in the new API, the inset range encodes two unrelated pieces of information: the line number where the inset should appear, and the height (in lines) of the inset. The fact that range.end.line - range.start.line encodes the height is unexpected. I think it would be clearer and more useful to provide two separate parameters: createWebviewTextEditorInset(editor, line, height, options).

Second, I think the height needs to be specified in either lines or pixels. Consider the Python case below (still a work in progress). A textual result is easiest to express in lines, but an image result is easiest to express in pixels. Indeed, I cannot find an extension API method to get the line height to do the conversion myself. If such an API call exists, then I suppose it's okay to make the extension writer do the conversion.

image

@jrieken
Copy link
Member

jrieken commented Jun 20, 2019

the fact that range.end.line - range.start.line encodes the height is unexpected.

Took a short cut there... The neat thing with hight in lines (vs pixels) is that is honours different line height settings without further ado. Not yet sure how we could expose that to extension author tho...

jrieken added a commit that referenced this pull request Jun 20, 2019
@jrieken
Copy link
Member

jrieken commented Jun 20, 2019

latest proposal (height is still a multiple of line numbers)

export function createWebviewTextEditorInset(editor: TextEditor, line: number, height: number, options?: WebviewOptions): WebviewEditorInset;

kiku-jw added a commit to kiku-jw/vscode that referenced this pull request Jun 26, 2019
* beautify macos keyboard layout label

* Open folders and workspaces in new windows

* Basic file opening via Open File command

* Update auto detect layout info.

* Respect openFoldersInNewWindow setting for folders/workspaces

* Make openWindow function resolve at right time

* keyboard layout status bar item tooltip

* Move workspace menu and action to fileActions.contribution

* Add clarifying comment on instance service request events

* Fullscreen change event.

* Remove unneeded margin on settings editor scrollbar
Fix microsoft#75724

* fix: microsoft#72626

* Remove extra register of automatic tasks

Fixes microsoft#75758

* remove trailing '/' from repo url for baseFolderName

* handle style-attribute modifications, cache requests in addition to results, microsoft#75061

* fix microsoft#75818

* fix bad tree guide indentation

* remove TODO

* update eslint

* update distro

fixes microsoft#73872

* Revert "Revert "Merge pull request microsoft#75695 from orange4glace/master""

This reverts commit a05e05c.

* Revert "Revert "explorero: file actions disablment no longer needed""

This reverts commit b634152.

* more code insets API tweaks, microsoft#66418

* Alpine build

* Update distro hash

* Remove duplicate cp

* shellscript: Add folding markers

* fixes microsoft#75829

* show setting on windows only

* add ExtensionKind and remoteName propsed APIs, microsoft#74188

* debt - use file service based configuration file service

* fix tests

* debt create configuration file service inside configuration service

* First cut of file service based user data service

* Use user data service for reading settings

* Update distro hash

* add diagnostic tool for git file event issues

* 💄

* Update distro hash

* introduce VSCODE_STEP_ON_IT

* remove env scripts

fixes microsoft#74792

* Update xterm.css

Fixes microsoft#75827

* check if file exists

* remove alert, aria-live will read the content even with no focus

fixes microsoft#41356

* win code.sh fix

* 🧀 Fix microsoft#75831

* Add proposed api check for shell API

Part of microsoft#75091

* launch ext host window internally

* EH debugging: support multiple files and folders

* Update distro

* xterm@3.15.0-beta50

Diff: xtermjs/xterm.js@846a189...96eafd3

Changes:

- Publish improvements
- Layering/strict updates

* Fire onDidChangeMaximumDimension when dimensions are set

Fixes microsoft#73496

* Fix potential race

* Delete cached service worker entries after a short timeout

* Fix webview developer command not being registered

* Re-queue canceled geterr requests before remaining buffers

We should give higher priority to files that have previously had geterr triggered on them but did not have their request completed

* Remove log uploader

Fixes microsoft#75748

* Use localized name for macOS keyboard layout

* fixes microsoft#75856

* User keyboard layout

* simplify common keymap layer

* load user keyboard layout after initialization

* US Standard keyboard info

* better score for layout

* fast return keyboard layout if 48-keymap matches

* a single keyboard event can be a keymap

* switch to user selected keyboard layout

* Have `.get` return promise directly

* Make sure we wait until service worker is ready before creating content

* Add version check to service worker

Try to make sure our page is talking to the expected version of the service worker

* Don't use clone as much

* Move host javascript to own file

* Update distro

* Remove icon explorations before shipping stable

* Move listener to window service.

* Minimap: Render find match decorations, fixes microsoft#75216

* Fix `navigator.serviceWorker.ready` is a Promise, not a function

* Use update instead of manually tring to re-register

* Extract ITypeScript server interface

* extract server error to own file

* Extract server spanwer to own file

* Renames

* Move getQueueingType into class

* Add experimental dual TS server

Fixes microsoft#75866

* Enable "typescript.experimental.useSeparateSyntaxServer" for VS Code workspace

* Remove trailing comma

* Include server id in TS server errors

* Make execute command a configuration object

* Also include format in the syntax commands

* Fix method name

* Renames

* Better encapsulate logic of spawning different server kinds

* some fixes for mac web

* New test runner API for microsoft#74555

* update doc, microsoft#74188

* build: release only iff all builds succeed, introduce VSCODE_RELEASE env

* first version of vscode.workspace.fs

* 💄

* Tasks registration + the local ext host now has an autority

Part of microsoft/vscode-remote-release#757

* Add platform override to getDefaultShellAndArgs in terminal

Part of microsoft/vscode-remote-release#757

* Ensure no trailing path separtor on URIs from file picker

Part of microsoft#75847

* data tree view state should store scrollTop, microsoft#74410

* fix microsoft#75564

* Change promise structure of creating terminal in tasks

Potential fix for microsoft#75774

* do not allow additionalProperties

microsoft#75887

* explorer: roots forget children on new file system provider registration

microsoft#75720

* Update max tokenization limit without reload

* Use interfaces for keyboard layout registration

* Separate keyboard layout loading logic for testing

* Test browser keymapper

* unused standard keyboard event.

* Make sure we dismiss the zoom status bar entry when switching editors

* Reduce state

* Added strictly typed telemetry function (microsoft#75915)

* Added strictly typed telemetry function

* cleanup publicLog2 signature

* Extract port mapping helper function

* Re-use extractLocalHostUriMetaDataForPortMapping for openUri

* Also map 127.0.0.1 in webviews and forward it for openExternal

Fixes microsoft/vscode-remote-release#108

* use empty model when content is empty

* 💄

* Update keyboard layout file comments

* Delete breadcrumbs.filterOnType unused setting. Fixes microsoft#75969

* Add quick open/input color registrations (fixes microsoft#65153)

* Update API

* implements ExtHostEditorInsetsShape

* use divs for tree indent guides

fixes microsoft#75779

* comment out more (for microsoft#74898)

* Quick Open > Quick Input (microsoft#65153)

* build - enable language server tests again (for microsoft#74898)

* use polish for wsl1

* move extension kind to Extension-interface

* init log level of remote log service

* Open/Save local commands should not show in the command palette

Fixes microsoft#75737

* chockidar: use polling

* fix build conditions

* xterm fixes for cglicenses

* oss 1.36.0

* workaround for microsoft#75830

* update distro commit

* electron - still call setBounds() as workaround for first window

* fixes microsoft#75753

* node-debug@1.35.3

* remove user data service

* use posix.join

* update doc

* Add -1 tab index to status bar entries

This keeps them out of the tab order, but allows them to be read with a screen reader

Fixes microsoft#41406

* empty view polish labels for remote case

microsoft/vscode-remote-release#511

* send remote watcher error to file service (fixes microsoft/vscode-remote-release#329)

* update distro

* better error handling in case of loader error in tests

* fix win 32 bits unit tests

* electron@4.2.5 (microsoft#76020)

* Code-insiders started from WSL doesn't return to console/ doesn't connect. Fixes microsoft/vscode-remote-release#780

* Group decorations by line before rendering

* disable support for simple fullscreen (microsoft#75054)

* telemetry - add window.nativeFullScreen

* move API to stable,  microsoft#74188

* build - add and use --disable-inspect for integration tests (microsoft#74898)

* 💄

* bump distro

* Report workspace stats in shared process

* Make return undefined explicit

* Add missing return

* Use explicit window.createWebviewManager

* gdpr comments

* webkit fullscreen detection

* Fix file name spelling

* update distro

* add logging

* disabling installing extension from gallery when not enabled

* status.workbench.keyboardLayout

* Move Inspect Keyboard Layout JSON to workbench

* return local extension after install

* install deps and packs while installing from gallery

* Fix default shell selector outside of Windows

Fixes microsoft#76040

* Add explicit win32 gheck for using user specific temp folder

* Always use settings UI when querying online services, fixes microsoft#75542

* Disable conpty in terminal when accessibility mode is on

Fixes microsoft#76043

* Move the webviewResourceRoot property to be set on each webview instead of as a global property

For microsoft#72155

This allows  us to potentially change the resource root per webview

* Make RelativeWorkspacePathResolver  a static class

* Use openExternal

* Auto restart when changing  typescript.experimental.useSeparateSyntaxServer

* Fix regular expression for rewriting iframe webview html replacing quotes

* Telemetry Command (microsoft#76029)

* Added telemetry command

* Initial Build support

* Added build logic for telemetry

* Linux Builds

* Windows builds sort of work

* Remove arm telemetry extraction

* Remove alpine telemetry extraction

* Remove accidental s

* More try catch

* Use full resource uri for transforming webview resources

This ensures we still work even if there is no base uri set

* Use outerHtml to make sure we write `<html>` element from extensions too

* Use a regexp that works across browsers

* Implement reload on iframe based webview Elements

* fix various nls issues

* 💄

* add debug output (microsoft#76024)

* Fix tasks platform for quoting

Fixes microsoft#75774

* fix hockeyapp symbols and report errors (fix microsoft#76024)

* update distro

* fix bad watch

* update distro

* Fix drive letter casing on typescript tasks

Occurs when opening by double clicking on workspace file. Fixes microsoft#75084

* update distro

* update distro

* Test remoteName and extensionKind (for microsoft#76028)

* MainThreadFileSystem does not await

* Fix microsoft#76096

* Rename runInBackground to hideFromUser

See microsoft#75278

* Update distro

* Fix minimap decoration rendering on horizontal scroll, fixes microsoft#76128

* Handle windows paths correctly when loading webvie resources

* Fix standard link handler for iframe based webviews

* Mark extensions.all as readonly

This iteration, we marked a few other arrays as readonly. We should do the same for extensions.all

* Fix microsoft#75927.

* Register mouse down on container dom node.

* Make sure we never cancel a request to just one of the ts servers

Fixes microsoft#76143

* Show document link tooltip first and put click instructions in parens

Fixes microsoft#76077

This change also update our standard link hovers to follow this format

* reset listener once users choose a dedicated keyboard layout

* switch to a new layout only when the score is higher.

* Fix kb unit test

* fix microsoft#76149

* web - document some API

* 💄 workbench API

* disable arm and alpine for stable

fixes microsoft#76159

* Fix extra auto complete on fast delete (microsoft#74675)

Fixes #vscode-remote-release/4

* use yarn --frozen-lockfile for builds

* remove `update.enableWindowsBackgroundUpdates` from online settings

* fix microsoft#76076

* revert the change

* prevent product.json containing gallery

* fix microsoft#76074

* fixes microsoft#54084

* Fix microsoft#76105

* fix microsoft#75904

* workaround for  microsoft#74934
@arash-hacker
Copy link

arash-hacker commented Aug 18, 2019

how access createWebviewTextEditorInset inside vscode .
i update my vscode package but doesn't appear this method?
my vscode.d.ts is not update when install vscode ,@types/vscode and vscode-test
thanks

@dddom
Copy link

dddom commented Oct 1, 2019

Hello, maybe I don't fully understand how this works, but is there a piece of API documentation relevant to this functionality, or is it not fully completed?

@samghelms
Copy link

@melvinroest
Copy link

Wait, I don't get it. I'm really confused.

Can I now use the //INLINE <img_url> command?

(In my code editor it doesn't seem to work)

I rather don't build a Python VSCode extension that is 20+ commits ahead and 300+ commits behind when I'm coding in JavaScript.

@rdeline rdeline deleted the code-inset branch October 15, 2019 17:29
@yozlet
Copy link

yozlet commented Oct 18, 2019

@dddom: The API is still in Proposed status, which means it can't be used in extensions yet, but that's usually a matter of time. You can see the implementation of window.createWebviewTextEditorInset here and what it provides here.

@melvinroest No, the "//INLINE" comment is an example of an extension which could be built to use Code Insets, rather than a description of this feature. So far, there are no user-visible changes to VSCode, and no published extensions can use Code Insets until it leaves Proposed status.

@techsin
Copy link

techsin commented Nov 25, 2019

hi guys complete noob here, is this in production now?

@tamuratak
Copy link
Contributor

FYI: issue #85682 opened.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet