Adds editor options overloads to showTextDocument & vscode.diff by eamodio · Pull Request #23641 · microsoft/vscode

eamodio · 2017-03-30T06:35:14Z

This PR hopefully addresses #10568
Also addresses a need I have with vscode.diff to have the same control (for GitLens)

@jrieken This is a first shot at this -- please let me know what you think. I added it as options so that it could be expanded later to allow for other editor options (but didn't want to go there at this stage).

mention-bot · 2017-03-30T06:35:16Z

@eamodio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @alexandrudima and @jrieken to be potential reviewers.

msftclas · 2017-03-30T06:58:14Z

@eamodio, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

jrieken · 2017-03-30T06:59:40Z

Check, in the middle of endgame this week. We have resources to jump on this starting next week

eamodio · 2017-03-30T07:02:36Z

OK, thanks. I'll dig into the broken tests soon as well

eamodio · 2017-04-09T15:13:22Z

@jrieken I resynced this with master and ran the test locally and I don't get any failures. Hopefully the builds will succeed now.

eamodio · 2017-04-12T04:48:34Z

I see the failures now (in the integration tests), but I'm having trouble figuring out how to debug them. Any help would be appreciated.

jrieken · 2017-04-13T15:13:46Z

src/vs/vscode.d.ts

Sooo, if we add this new overload it should include column and also make it a type which we can document nicer

OK - Will do. Should I also allow any other editor options come through? I have it explicitly locked to just that set right now

jrieken · 2017-04-13T15:14:32Z

src/vs/workbench/api/node/extHost.protocol.ts

In the protocol/shape there should only be one signature and the calling side should do the argument construction

jrieken · 2017-04-13T15:18:27Z

Please also rebase this on master to see if those tests still fail, they shouldn't. Then I wonder if we actually tackle #10568 here. There is also an ask to (1) know if an editor is pinned or not pinned and (2) an ask to change that after the fact. We should think about exposing this information on the editor, similar to viewColumn, and about allowing to change that property, similar to changing a selection. However, important is to know that there are embedded and other editors that cannot be pinned etc. Last, we should think of a better name than 'pinned'.

eamodio · 2017-04-13T16:17:27Z

For a name instead of pinned -- how about the inverse - preview since I believe that's what its called in the docs (preview tab)?
Did you paste the wrong link (or am I missing something) because I thought this does address #10568.

As for exposing the information -- totally agree, I look into adding that. But for changing after the fact -- that feels less clear. Should you be able to go from something pinned to not pinned? Maybe so, but the UI currently doesn't provide a way for that direction (though an extension might have a good reason for it). What do you think?

Also can you elaborate on the last point about certain editors can't be pinned -- where would I find that, and would/should that be handled explicitly here?

jrieken · 2017-04-13T16:21:33Z

Look here (#10568 (comment)) which is about making the transition. Basically turning an existing editor into a pinned (from a preview editor)

Also can you elaborate on the last point about certain editors can't be pinned

This is only an issue when the pinned-state is exposed as property. Similar to viewColumn is only applies to one the "big editors" not those of a peek view (find references) or the left of a diff editor

eamodio · 2017-04-13T18:09:25Z

Ah got it -- going from unpinned --> pinned make perfect sense (was thinking that could just be handled by another call to showTextDocument, but something more explicit is probably best). Think it should be a 1-way deal, or also support going from pinned --> unpinned?

eamodio · 2017-04-13T18:52:30Z

I made the first set of changes -- didn't rename pinned yet until there is agreement on a new name.
Also haven't looked into adding pinned as a property yet.
Since we moved column into the options, I added column support to the diff command too.

eamodio · 2017-04-13T18:53:00Z

src/vs/vscode.d.ts

@jrieken I don't love this name, let me know what you think

Maybe turning it around TextDocumentShowOptions or TextDocumentViewOptions?

TextDocumentDisplayOptions?

not sure yet.

eamodio · 2017-04-13T18:55:05Z

src/vs/workbench/api/node/mainThreadEditors.ts

@jrieken I moved some of the data massaging into here, so that it could take the same ShowTextDocumentOptions (since I think you can technically call the proxy methods from inside an extension).

Don't use the TypeConverter here but send proper data. Do all the massaging before calling a main-side method. Rule of thumb is that mainXYZ files shouldn't depend on extHostXZY files

The main issue with that, is then we would need 2 "versions" of the TextDocument[Show|View|Display]Options -- one for the extensions with a column and one for the main-side with a position. Which is certainly doable, I was just trying to not have 2 types with almost the same fields. Are you fine with adding 2 types? And if so, where should the main-side one live and how should it be named to not cause confusion?

Two - one is API the other the DTO

jrieken · 2017-04-24T07:20:38Z

@eamodio Can you go ahead and resolve the merge conflicts?

jrieken · 2017-04-24T07:21:33Z

src/vs/vscode.d.ts

Maybe turning it around TextDocumentShowOptions or TextDocumentViewOptions?

jrieken · 2017-04-24T07:23:16Z

src/vs/workbench/api/node/mainThreadEditors.ts

Don't use the TypeConverter here but send proper data. Do all the massaging before calling a main-side method. Rule of thumb is that mainXYZ files shouldn't depend on extHostXZY files

eamodio · 2017-04-24T12:50:30Z

@jrieken is a rebase/force push fine to resolve the conflicts or should I do it in a follow-on commit?

jrieken · 2017-04-24T13:02:14Z

rebase/force push

That's OK for me

eamodio · 2017-04-24T13:39:06Z

@jrieken Are you good with preview as the name rather than pinned (even though it is the inverse)?

Also see comments above

jrieken · 2017-04-24T14:00:18Z

Yeah, let go with preview for now

Moves column into the options Removes overloads from the proxy

Changes pinned property to preview Adds ITextDocumentShowOptions as main-side DTO

eamodio · 2017-04-25T13:34:35Z

@jrieken I've resolved the conflicts and made all the code review changes above

jrieken · 2017-04-25T13:56:23Z

src/vs/vscode.d.ts

+	export interface TextDocumentShowOptions {
+		/**
+		 * An optional view column in which the [editor](#TextEditor) should be shown. The default is the [one](#ViewColumn.One), other values are adjusted to be __Min(column, columnCount + 1)__.
+		 */


Wow - your commit should have been blocked because that's a formatting issue...

jrieken · 2017-04-25T13:58:11Z

src/vs/workbench/electron-browser/commands.ts

 	});

-	CommandsRegistry.registerCommand('_workbench.diff', function (accessor: ServicesAccessor, args: [URI, URI, string, string]) {
+	CommandsRegistry.registerCommand('_workbench.diff', function (accessor: ServicesAccessor, args: [URI, URI, string, string, vscode.TextDocumentShowOptions]) {


it's dangerous to use the vscode-module here (actually everywhere outside the extension host) because these type actually don't exist. for interfaces it's OK because they disappear but using IEditorOption would have been better

I don't see an IEditorOption -- there is IEditorOptions, but that interface doesn't have any of the fields. Are you suggesting adding them there? Or am I missing something?

jrieken · 2017-04-25T14:03:24Z

Trying to understand why the test have failed...

jrieken · 2017-04-25T14:05:50Z

src/vs/workbench/api/node/extHostTextEditors.ts

+				pinned: true
+			};
+		} else {
+			options = {


The else cannot be unconditional because it will be hit when no second parameter is given. Add a check like typeof columnOrOptions === 'object'

eamodio · 2017-04-25T14:50:40Z

I think the fall-through case was causing the test failures -- they all passed now! :)

jrieken · 2017-04-25T14:57:26Z

Merging now. Thanks for this PR, for reacting to feedback.

eamodio · 2017-04-25T14:59:59Z

@jrieken thank you!

jrieken · 2017-04-25T15:31:52Z

fyi - I have made some minor tweak: align vscode.diff-options with showTextDocument, doc string, and I have renamed column to viewColumn so that it is aligned with the editor property. See: 9f01642, 936104d, and 779e7af

eamodio · 2017-04-25T16:06:50Z

Look great -- just had that 1 comment on a possible behavior change.

msftclas added the cla-required label Mar 30, 2017

msftclas added cla-signed and removed cla-required labels Mar 30, 2017

jrieken self-assigned this Mar 30, 2017

jrieken added this to the April 2017 milestone Mar 30, 2017

eamodio force-pushed the feature-show-editor-options branch from f789b62 to 9dd64f7 Compare April 9, 2017 15:12

jrieken requested changes Apr 13, 2017

View reviewed changes

eamodio force-pushed the feature-show-editor-options branch from 9dd64f7 to 1c92003 Compare April 13, 2017 18:49

eamodio commented Apr 13, 2017

View reviewed changes

jrieken added api feature-request Request for new features or functionality labels Apr 14, 2017

jrieken requested changes Apr 24, 2017

View reviewed changes

jrieken added this to the May 2017 milestone Apr 25, 2017

jrieken removed this from the April 2017 milestone Apr 25, 2017

eamodio added 4 commits April 25, 2017 08:45

Adds editor options overload to showTextDocument

b5340db

Adds editor options overload to vscode.diff

f47a862

Adds ShowTextDocumentOptions interface

d44fa8f

Moves column into the options Removes overloads from the proxy

Changes ShowTextDocumentOptions to TextDocumentShowOptions

ca163e0

Changes pinned property to preview Adds ITextDocumentShowOptions as main-side DTO

eamodio force-pushed the feature-show-editor-options branch from 1c92003 to ca163e0 Compare April 25, 2017 13:33

jrieken approved these changes Apr 25, 2017

View reviewed changes

jrieken requested changes Apr 25, 2017

View reviewed changes

eamodio added 2 commits April 25, 2017 10:13

Fixes formatting issue

f3963ec

Fixes fallthrough with no options

c5de5b2

jrieken modified the milestones: April 2017, May 2017 Apr 25, 2017

jrieken approved these changes Apr 25, 2017

View reviewed changes

jrieken merged commit 7ecebac into microsoft:master Apr 25, 2017

jrieken added verification-needed Verification of issue is requested and removed verification-needed Verification of issue is requested labels Apr 25, 2017

This was referenced Apr 25, 2017

Let showTextDocument always create a new tab #9138

Closed

How to make a TextDocument to be *pinned*? #10568

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

eamodio commented Mar 30, 2017

Uh oh!

mention-bot commented Mar 30, 2017

Uh oh!

msftclas commented Mar 30, 2017

Uh oh!

jrieken commented Mar 30, 2017

Uh oh!

eamodio commented Mar 30, 2017

Uh oh!

eamodio commented Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eamodio commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Apr 13, 2017

Uh oh!

eamodio commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrieken commented Apr 13, 2017

Uh oh!

eamodio commented Apr 13, 2017

Uh oh!

eamodio commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eamodio commented Apr 24, 2017

Uh oh!

jrieken commented Apr 24, 2017

Uh oh!

eamodio commented Apr 24, 2017

Uh oh!

jrieken commented Apr 24, 2017

Uh oh!

eamodio commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

eamodio commented Apr 9, 2017 •

edited

Loading

eamodio commented Apr 13, 2017 •

edited

Loading

jrieken Apr 25, 2017 •

edited

Loading

jrieken commented Apr 25, 2017 •

edited

Loading