Open a previewEditor while navigating files through QuickOpen #13236

XVincentX · 2016-10-05T08:03:20Z

The following PR will solve #12892 - allowing to have a preview editor while navigating into files from a QuickOpen menu.

mention-bot · 2016-10-05T08:03:22Z

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

msftclas · 2016-10-05T08:03:24Z

Hi @XVincentX, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

bpasero · 2016-10-05T08:11:45Z

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

@@ -404,7 +404,7 @@ export class EditorPart extends Part implements IEditorPart, IEditorGroupService
 			}

 			// Focus (unless prevented)
-			const focus = !options || !options.preserveFocus;
+			const focus = !options || options.preserveFocus;


This cannot be changed...

Ok, effectively we're not really taking about preserving focus at the end, but if give focus to the new opened editor - which is a different option. Now I understand that.

Anyway, if the options object is undefined it's going to focus the thing anyway - how should we handle this situation?

that is the correct behaviour. keeping focus should be the exception so please leave it like that

bpasero · 2016-10-05T08:24:33Z

src/vs/workbench/browser/quickopen.ts

+
+		if (input instanceof EditorInput) {
+			if (isPreview) {
+				options.preserveFocus = false;


bpasero · 2016-10-05T08:24:48Z

src/vs/workbench/common/editor.ts

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


all of this cannot be changed...

XVincentX · 2016-10-05T08:31:02Z

@bpasero Cool - I misinterpreted the meaning of preserveFocus
I've flipped the behaviours and now it works correctly!

Thanks for the patience - I'll keep going and see what I can find out for the history entry!

coveralls · 2016-10-05T08:51:16Z

Changes Unknown when pulling b839e72d86be47cd181a0cc958f2764602607256 on XVincentX:master into * on Microsoft:master*.

coveralls · 2016-10-05T08:57:06Z

Changes Unknown when pulling b839e72d86be47cd181a0cc958f2764602607256 on XVincentX:master into * on Microsoft:master*.

coveralls · 2016-10-05T16:32:19Z

Coverage decreased (-0.004%) to 61.23% when pulling def22b0a364b57ff956dc1be011a2470307bfec0 on XVincentX:master into 809cef1 on Microsoft:master.

coveralls · 2016-10-05T17:13:16Z

Coverage decreased (-0.01%) to 61.222% when pulling d19c3d539f4550a02866554369f5e58654331b49 on XVincentX:master into 809cef1 on Microsoft:master.

coveralls · 2016-10-05T17:26:02Z

Coverage decreased (-0.01%) to 61.221% when pulling 7296d6a3c11be727d64e5bdb3022ae318b1e2d5a on XVincentX:master into 809cef1 on Microsoft:master.

coveralls · 2016-10-05T17:59:35Z

Coverage decreased (-0.01%) to 61.222% when pulling 7296d6a3c11be727d64e5bdb3022ae318b1e2d5a on XVincentX:master into 809cef1 on Microsoft:master.

bpasero · 2016-10-07T06:46:26Z

@XVincentX I am not 100% convinced that this should be an editor option. It seems very specific to the history management so maybe it would be better to just add a method to the history service to temporarily ignore editor changes while you navigate through the results.

There are 2 ugly cases that we still need to figure out: If you check for listeners of onEditorOpening you can find:

the remove from editor history action that waits for a selection being made in quick open to do something (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/editorActions.ts#L1168)
the "compare active file with" action that does a similar trick (https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/files/browser/fileActions.ts#L1248)

Obviously those two would not work anymore if we start to preview editors while navigating. i wonder if another solution should be added to avoid the onEditorOpening event hack.

bpasero · 2016-10-07T06:52:01Z

I think the right approach to solve this issue is to change the IQuickOpenService.show() method to allow to be run in a mode where the result is returned, e.g. by passing in a function that should be called when an element is selected. If this is provided, the default action in the run method would not be called. That needs quite some adoption though across quick open providers...

XVincentX · 2016-10-07T16:33:36Z

@bpasero I see.
I am totally willing to contribute! However if we do not really have a concrete action plan (agreed by you and probably your team mates) I have no idea how to start and make sure I do not come up with wrong solutions.

bpasero · 2016-10-13T11:58:51Z

@XVincentX I pushed a change to get rid of onEditorOpening because I felt it was very ugly. This should make your PR simpler now. I think you should push for a method in history service to simply ignore pushing to the stack and try to leverage that 👍

XVincentX · 2016-10-13T12:00:22Z

@bpasero Could you link the commit/change you made? So I can understand what happened in particular.

bpasero · 2016-10-13T12:04:47Z

@XVincentX here: dfd3f57

This event would always fire when an editor is about to open and it would interfere with your contribution to open editors while navigating over results in the picker. I was never fond of that event and now decided to get rid of it. This should make your life easier.

XVincentX · 2016-10-13T12:11:03Z

Thanks! Let’s see if I can come up with something. V.

XVincentX · 2016-10-16T18:58:36Z

@bpasero Unfortunately I will not be able to work on this during this week as I'll be out for the entire week. Please bear with me, hopefully I should be able to get the hands on next week.

bpasero · 2016-10-17T04:58:50Z

@XVincentX np at all 👍 . To get it into this month release we would need to finish it on Monday though as we are closing then for finishing the sprint.

bpasero · 2016-10-26T04:51:50Z

Closing in favour of #13909

msftclas added the cla-already-signed label Oct 5, 2016

bpasero reviewed Oct 5, 2016

View reviewed changes

src/vs/workbench/browser/quickopen.ts

if (input instanceof EditorInput) {

if (isPreview) {

options.preserveFocus = false;

Copy link

Member

bpasero Oct 5, 2016

Choose a reason for hiding this comment

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

flip this

bpasero reviewed Oct 5, 2016

View reviewed changes

bpasero self-assigned this Oct 5, 2016

XVincentX changed the title ~~WIP - Open a previewEditor while navigating files through QuickOpen~~ Open a previewEditor while navigating files through QuickOpen Oct 5, 2016

XVincentX added 2 commits October 7, 2016 18:14

Open the editor even when PREVIEW is selected.

e106d29

Set preserveFocus to true when opening a previewEditor

322edd9

bpasero closed this Oct 26, 2016

shreeve mentioned this pull request Feb 16, 2019

Preview files in quick open and when highlighting in explorer shreeve/vscode#1

Open

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

Open a previewEditor while navigating files through QuickOpen #13236

Open a previewEditor while navigating files through QuickOpen #13236

XVincentX commented Oct 5, 2016

mention-bot commented Oct 5, 2016

msftclas commented Oct 5, 2016

bpasero Oct 5, 2016

XVincentX Oct 5, 2016 •

edited

Loading

bpasero Oct 5, 2016

bpasero Oct 5, 2016

bpasero Oct 5, 2016

XVincentX commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

bpasero commented Oct 7, 2016 •

edited

Loading

bpasero commented Oct 7, 2016

XVincentX commented Oct 7, 2016

bpasero commented Oct 13, 2016 •

edited

Loading

XVincentX commented Oct 13, 2016

bpasero commented Oct 13, 2016

XVincentX commented Oct 13, 2016 via email

XVincentX commented Oct 16, 2016

bpasero commented Oct 17, 2016 •

edited

Loading

bpasero commented Oct 26, 2016

Open a previewEditor while navigating files through QuickOpen #13236

Open a previewEditor while navigating files through QuickOpen #13236

Conversation

XVincentX commented Oct 5, 2016

mention-bot commented Oct 5, 2016

msftclas commented Oct 5, 2016

bpasero Oct 5, 2016

Choose a reason for hiding this comment

XVincentX Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

bpasero Oct 5, 2016

Choose a reason for hiding this comment

bpasero Oct 5, 2016

Choose a reason for hiding this comment

bpasero Oct 5, 2016

Choose a reason for hiding this comment

XVincentX commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

coveralls commented Oct 5, 2016

bpasero commented Oct 7, 2016 • edited Loading

bpasero commented Oct 7, 2016

XVincentX commented Oct 7, 2016

bpasero commented Oct 13, 2016 • edited Loading

XVincentX commented Oct 13, 2016

bpasero commented Oct 13, 2016

XVincentX commented Oct 13, 2016 via email

XVincentX commented Oct 16, 2016

bpasero commented Oct 17, 2016 • edited Loading

bpasero commented Oct 26, 2016

XVincentX Oct 5, 2016 •

edited

Loading

bpasero commented Oct 7, 2016 •

edited

Loading

bpasero commented Oct 13, 2016 •

edited

Loading

bpasero commented Oct 17, 2016 •

edited

Loading