Addresses #26184 - use configuration for pinned default #27357

eamodio · 2017-05-27T03:30:28Z

Uses the configuration setting workbench.editor.enablePreview to provide a default of pinned if not explicitly passed. See #26184 (comment)

Hopefully this is decent way of implementing this :)

/cc @jrieken @bpasero

mention-bot · 2017-05-27T03:30:30Z

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

jrieken · 2017-05-28T09:34:03Z

I have the feeling that this would be a lot more simple if that is implemented in the _workbench.diff-command with some true, false, and undefined values for pinned set from the API

eamodio · 2017-05-28T17:23:43Z

@jrieken I've re-worked this completely to move it down to hopefully the lowest level. So as per @bpasero comments, if you pass true / false for pinned it will be honored, but undefined will enact the default behavior which is controlled first by workbench.editor.enablePreview and then the existing other checks

bpasero · 2017-05-29T04:46:46Z

@eamodio this is a bad change as it can result in editors showing up as preview even though "workbench.editor.enablePreview": is set to false as soon as a client of openEditor is setting pinned: false.

I think what Joh meant is to implement this behaviour only in the _workbench.diff command but not throughout all users in the workbench?

eamodio · 2017-05-29T04:54:14Z

@bpasero I guess I misunderstood your original comments. I thought the desire was to allow extensions to be able to fully control the pinned state regardless of workbench.editor.enablePreview, but if the extension passed undefined then the default would be based upon the current default rules including the check of workbench.editor.enablePreview.

If that isn't the case and workbench.editor.enablePreview should rule regardless I can just tweak that back here to make the this.tabOptions.previewEditors always win

eamodio · 2017-05-29T05:14:54Z

@bpasero Updated to honor workbench.editor.enablePreview above all else

jrieken · 2017-06-09T10:07:12Z

Not sure how this should work @bpasero? Now pinned is more or less removed in most placed which makes sense if we have a reasonable and configurable default but I don't know why folks have done it like that in the first place

bpasero · 2017-06-09T17:16:13Z

I do not really understand the need for introducing undefined for this. I agree that we should not set pinning hard coded from anywhere unless we know that the action that triggers the opening wants to pin or not pin. One example is double clicking on an entry in a tree or making something dirty.

If we simply do not set pinning state when opening things, the default will be to open as preview unless this is disabled via user settings.

In other words, I would let extensions opt-in to pinning (with the API we have today) but not set it by default. This should give the correct experience.

Btw for webviews we should probably enforce pinning simply because the UI state in there is not preserved and it would be odd to loose the content so quickly.

eamodio · 2017-06-09T17:27:06Z

When I wasn't honoring workbench.editor.enablePreview, undefined was required as there wasn't a way to say I want the "default" behavior, its either pinned or not. But now that we are again always honoring workbench.editor.enablePreview I guess pinned === false is the equivalent of use the "default".

Although honestly, I'd rather leave undefined in there because it is a clear indicator of not specifying the behavior and that the default behavior should be used. That way if down the line something there changes you know the difference between something not caring about pinned/unpinned vs explicit intent.

bpasero

-1 for using undefined everywhere. +1 for not setting preview from extensions unless explicitly set.

eamodio · 2017-06-13T04:55:48Z

@bpasero do you want me to make the changes to take out undefined?

bpasero · 2017-06-13T05:00:31Z

@eamodio yes, thanks

re: requested code review changes

eamodio · 2017-06-20T03:34:37Z

@bpasero I (hopefully) removed all the undefined's from the internal calls -- though I left it from calls from extensions. I also rebased this branch to master as well. Let me know if you want me to make any other changes.

bpasero

@eamodio Thanks, I think we can still reduce the number of changes.

bpasero · 2017-06-20T05:27:35Z

src/vs/workbench/browser/parts/editor/editorPart.ts

@@ -319,7 +319,14 @@ export class EditorPart extends Part implements IEditorPart, IEditorGroupService
 		// while the UI is not yet ready. Clients have to deal with this fact and we have to make sure that the
 		// stacks model gets updated if any of the UI updating fails with an error.
 		const group = this.ensureGroup(position, !options || !options.preserveFocus);
-		const pinned = !this.tabOptions.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();
+
+		let pinned = !this.tabOptions.previewEditors;


Can we leave this as:

const pinned = !this.tabOptions.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();

I see now reason to change it and find the new code unreadable.

This allows pinned: false to be honored explicitly rather than it falling through as a default.

@eamodio I am not sure we can do that, e.g. some code relies on the fact that dirty and untitled files open as pinned and never as preview which would replace them once another preview opens.

bpasero · 2017-06-20T05:28:35Z

src/vs/workbench/common/editor.ts

@@ -589,7 +589,7 @@ export class TextEditorOptions extends EditorOptions {
 	public static from(input: IBaseResourceInput): TextEditorOptions {
 		let options: TextEditorOptions = null;
 		if (input && input.options) {
-			if (input.options.selection || input.options.viewState || input.options.forceOpen || input.options.revealIfVisible || input.options.revealIfOpened || input.options.preserveFocus || input.options.pinned || input.options.inactive || typeof input.options.index === 'number') {
+			if (input.options.selection || input.options.viewState || input.options.forceOpen || input.options.revealIfVisible || input.options.revealIfOpened || input.options.preserveFocus || input.options.pinned !== undefined || input.options.inactive || typeof input.options.index === 'number') {


Why are changes in this file needed, I suggest to leave it as it was?

This allows pinned: false to be honored explicitly rather than it falling through as a default.

Again, forcing pinned: false is not possible currently.

bpasero · 2017-06-20T05:29:00Z

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

@@ -401,7 +401,7 @@ export function registerCommands(): void {
 		if (!options || typeof options !== 'object') {
 			options = {
 				preserveFocus: false,
-				pinned: true
+				pinned: undefined


Remove pinned: undefined

bpasero · 2017-06-20T05:29:16Z

src/vs/workbench/common/editor.ts

@@ -614,8 +614,8 @@ export class TextEditorOptions extends EditorOptions {
 				options.preserveFocus = true;
 			}

-			if (input.options.pinned) {
-				options.pinned = true;
+			if (input.options.pinned !== undefined) {


Leave as it was?

Same as above

eamodio · 2017-06-27T21:19:39Z

@bpasero Any thoughts on the above?

bpasero · 2017-06-28T14:51:01Z

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

 			let editorOptions: IEditorOptions;
 			if (options) {
 				editorOptions = {
-					pinned: !options.preview,
+					pinned: options.preview === undefined ? undefined : !options.preview,


I better like: typeof options.preview === 'boolean' ? !options.preview : undefined

bpasero · 2017-06-28T14:51:29Z

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

 			};
 		} else if (typeof columnOrOptions === 'object') {
 			options = {
 				position: TypeConverters.fromViewColumn(columnOrOptions.viewColumn),
 				preserveFocus: columnOrOptions.preserveFocus,
-				pinned: columnOrOptions.preview === undefined ? true : !columnOrOptions.preview
+				pinned: columnOrOptions.preview === undefined ? undefined : !columnOrOptions.preview


I better like: typeof columnOrOptions.preview === 'boolean' ? ! columnOrOptions.preview : undefined

bpasero · 2017-06-28T14:52:32Z

src/vs/workbench/browser/parts/editor/editorPart.ts

@@ -319,7 +319,14 @@ export class EditorPart extends Part implements IEditorPart, IEditorGroupService
 		// while the UI is not yet ready. Clients have to deal with this fact and we have to make sure that the
 		// stacks model gets updated if any of the UI updating fails with an error.
 		const group = this.ensureGroup(position, !options || !options.preserveFocus);
-		const pinned = !this.tabOptions.previewEditors || (options && (options.pinned || typeof options.index === 'number')) || input.isDirty();
+
+		let pinned = !this.tabOptions.previewEditors;


@eamodio I am not sure we can do that, e.g. some code relies on the fact that dirty and untitled files open as pinned and never as preview which would replace them once another preview opens.

bpasero · 2017-06-28T14:52:53Z

src/vs/workbench/common/editor.ts

@@ -589,7 +589,7 @@ export class TextEditorOptions extends EditorOptions {
 	public static from(input: IBaseResourceInput): TextEditorOptions {
 		let options: TextEditorOptions = null;
 		if (input && input.options) {
-			if (input.options.selection || input.options.viewState || input.options.forceOpen || input.options.revealIfVisible || input.options.revealIfOpened || input.options.preserveFocus || input.options.pinned || input.options.inactive || typeof input.options.index === 'number') {
+			if (input.options.selection || input.options.viewState || input.options.forceOpen || input.options.revealIfVisible || input.options.revealIfOpened || input.options.preserveFocus || input.options.pinned !== undefined || input.options.inactive || typeof input.options.index === 'number') {


Again, forcing pinned: false is not possible currently.

bpasero · 2017-06-28T14:53:43Z

@eamodio what is the use case for forcing an editor to be not-pinned (= previewed)? Can you give me an example?

bpasero · 2017-07-05T14:59:18Z

@eamodio in your demo today at lunch, you indicated that you wanted to open diff editors from the picker as preview and not pinned. But is that really an issue still if we do not set the pinned state by default from extensions? It seems to me the original request for pinned: false only existed because we used to set pinned: true by default. If you would just not set the pinned attribute at all, any editor you open should show up as preview editor unless the user has disabled this feature.

eamodio · 2017-07-05T20:11:39Z

@bpasero Yeah, just being able to control it is enough. Though the main reason I wanted to plumb it all the way through was so that the final code could know the expressed desire of the extension, because it was passed as false, not left undefined. The only actual difference this would have is if this condition is met: (options && typeof options.index === 'number') || input.isDirty(), because those cases could end up changing pinned to true, even though false was passed. While this is true of the overall "preview editors" setting -- that feels different. IMO at least ;)

…fault

bpasero · 2017-07-06T06:14:02Z

@eamodio thanks. I made some changes on top of yours. My argument is still that the pinned state cannot be overridden by the user if explicitly defined. Lots of stuff in the editor world depends on a dirty file being pinned for example (you cannot have a dirty file that shows up as preview).

I went ahead and merged the PR because the original request seems addressed.

msftclas added the cla-already-signed label May 27, 2017

roblourens assigned bpasero May 28, 2017

bpasero assigned jrieken and unassigned bpasero May 28, 2017

eamodio force-pushed the feature-pinned-default branch 2 times, most recently from af5ddc3 to 14973d6 Compare May 28, 2017 17:20

This was referenced May 29, 2017

Closes #27410 - support multi-select on open file/changes commands #27411

Merged

Fixes #27408 - stops forcing column ONE as the default #27409

Closed

jrieken added this to the June 2017 milestone May 30, 2017

tibotiber mentioned this pull request Jun 7, 2017

Add preview support for diffEditor #27308

Closed

jrieken requested review from joaomoreno and bpasero June 9, 2017 10:06

jrieken assigned joaomoreno and bpasero Jun 9, 2017

bpasero requested changes Jun 13, 2017

View reviewed changes

eamodio added 3 commits June 19, 2017 22:47

Addresses microsoft#26184 - use configuration for pinned default

a6d1c46

Honors the preview editor setting above all

b7d6881

Removes passing undefined to pinned internally

c01e0ce

re: requested code review changes

eamodio force-pushed the feature-pinned-default branch from 00f4293 to c01e0ce Compare June 20, 2017 03:33

bpasero requested changes Jun 20, 2017

View reviewed changes

bpasero modified the milestones: On Deck, June 2017 Jun 20, 2017

Removes unnecessary undefined

3bdfe3c

bpasero requested changes Jun 28, 2017

View reviewed changes

bpasero added 2 commits July 6, 2017 07:56

Merge remote-tracking branch 'upstream/master' into feature-pinned-de…

0923e40

…fault

feedback

cf064df

bpasero merged commit 4f50b29 into microsoft:master Jul 6, 2017

bpasero modified the milestones: July 2017, On Deck Jul 6, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses #26184 - use configuration for pinned default #27357

Addresses #26184 - use configuration for pinned default #27357

eamodio commented May 27, 2017

mention-bot commented May 27, 2017

jrieken commented May 28, 2017

eamodio commented May 28, 2017

bpasero commented May 29, 2017

eamodio commented May 29, 2017

eamodio commented May 29, 2017

jrieken commented Jun 9, 2017

bpasero commented Jun 9, 2017

eamodio commented Jun 9, 2017

bpasero left a comment

eamodio commented Jun 13, 2017

bpasero commented Jun 13, 2017

eamodio commented Jun 20, 2017

bpasero left a comment •

edited

Loading

bpasero Jun 20, 2017

eamodio Jun 20, 2017

bpasero Jun 28, 2017

bpasero Jun 20, 2017

eamodio Jun 20, 2017

bpasero Jun 28, 2017

bpasero Jun 20, 2017

eamodio Jun 20, 2017

bpasero Jun 20, 2017

eamodio Jun 20, 2017

eamodio commented Jun 27, 2017

bpasero Jun 28, 2017

bpasero Jun 28, 2017

bpasero Jun 28, 2017

bpasero Jun 28, 2017

bpasero commented Jun 28, 2017

bpasero commented Jul 5, 2017

eamodio commented Jul 5, 2017

bpasero commented Jul 6, 2017

Addresses #26184 - use configuration for pinned default #27357

Addresses #26184 - use configuration for pinned default #27357

Conversation

eamodio commented May 27, 2017

mention-bot commented May 27, 2017

jrieken commented May 28, 2017

eamodio commented May 28, 2017

bpasero commented May 29, 2017

eamodio commented May 29, 2017

eamodio commented May 29, 2017

jrieken commented Jun 9, 2017

bpasero commented Jun 9, 2017

eamodio commented Jun 9, 2017

bpasero left a comment

Choose a reason for hiding this comment

eamodio commented Jun 13, 2017

bpasero commented Jun 13, 2017

eamodio commented Jun 20, 2017

bpasero left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eamodio commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpasero commented Jun 28, 2017

bpasero commented Jul 5, 2017

eamodio commented Jul 5, 2017

bpasero commented Jul 6, 2017

bpasero left a comment •

edited

Loading