quick open files in background #13925

wprater · 2016-10-18T07:55:47Z

allows one to continually open files while keeping quick open widget focused
uses the right modifier key to open in background
can open in split view if using the cmd trigger key

mention-bot · 2016-10-18T07:55:48Z

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

msftclas · 2016-10-18T07:55:51Z

Hi @wprater, 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!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

jrieken · 2016-10-18T08:02:49Z

@wprater There are compile errors - https://ci.appveyor.com/project/VSCode/vscode/build/1.0.9419#L2861

wprater · 2016-10-18T08:28:18Z

does TypeScript not support Object.assign? or do i need to alter a type def
file?
On Tue, Oct 18, 2016 at 1:03 AM Johannes Rieken notifications@github.com
wrote:

@wprater https://github.com/wprater There are compile errors -
https://ci.appveyor.com/project/VSCode/vscode/build/1.0.9419#L2861

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#13925 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAo1G12WTW6pxj_rayyJk2QB7iq8O--ks5q1H1PgaJpZM4KZeug
.

bpasero · 2016-10-18T08:33:41Z

@wprater we have a assign method in https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/objects.ts#L146 if you want to use that.

wprater · 2016-10-18T08:53:40Z

@wprater we have a assign method in https://github.com/Microsoft/vscode/blob/master/src/vs/base/common/objects.ts#L146 if you want to use that.

thanks. fixed up, but not sure why there are merge errors?

ps - on another note, I cannot seem to rid myself of this "ut8 dir path/file" test fixture, making it hard to rebase.

bpasero · 2016-10-18T09:36:24Z

src/vs/base/parts/quickopen/common/quickOpen.ts

@@ -43,7 +43,8 @@ export interface IAutoFocus {

 export enum Mode {
 	PREVIEW,
-	OPEN
+	OPEN,
+	OPEN_BEHIND


@wprater maybe better "OPEN_IN_BACKGROUND"?

I almost chose that name, but I felt it was like a background job. Will change.

bpasero · 2016-10-18T09:38:22Z

src/vs/base/parts/quickopen/browser/quickOpenWidget.ts

-			hide = this.model.runner.run(value, Mode.OPEN, context);
+			let mode = Mode.OPEN;
+
+			if (context.event instanceof KeyboardEvent) {


@wprater can we just send over a flag to indicate this mode from where we call this method with the arrow key right pressed? I think we already create the standard keyboard event there and do not need to create it again here.

how about we send over the StandardKeyboardEvent instead? I'd prefer this handler to account for biz logic as to which mode to use; elementSelected being called in multiple locations. StandardKeyboardEvent also has a browserEvent prop, so we can access the original event in this case.

Well you gotta be careful though because the event is being forwarded as context...

let me take another look at this. I can pass in the mode, if need be, or revert to the original event while in the context?

bpasero · 2016-10-18T09:43:18Z

src/vs/workbench/browser/quickopen.ts

+		if (mode === Mode.OPEN_BEHIND) {
+			opts = objects.assign(opts || {}, {
+				pinned: true,
+				revealIfVisible: true,


@wprater I think this option is a bit dangerous here because what it basically means is that the file will not be opened if it is visible in any other group. but I think the intent when sending files to open to the background is to open them in that group independent from other groups. I suggest to not set this flag.

good catch!

bpasero · 2016-10-18T09:44:40Z

src/vs/workbench/browser/quickopen.ts

 			} else {
+				objects.assign((<IResourceInput>input).options, opts);


@wprater carefull, options could be null within the resource input and I think this would break assign method.

@wprater also you seem to alter the way options used to be handled with IResourceInput. you are merging the options from getOptions() call into IResourceInput. we did not do that before though:

this.editorService.openEditor(<IResourceInput>input, sideBySide).done(null, errors.onUnexpectedError);

fixed up. Object.assign handles null sources fine, will review your implementation (objects.assign) better.

wprater · 2016-10-18T17:03:27Z

src/vs/base/parts/quickopen/browser/quickOpenWidget.ts

-			if (context.event instanceof StandardKeyboardEvent) {
-				if (context.event.keyCode === KeyCode.RightArrow) {
+			if (event instanceof StandardKeyboardEvent) {
+				eventForContext = event.browserEvent;


@bpasero preserving the browser's event as the context here. will this work better? allows us to leave Mode logic in elementSelected.

wprater · 2016-10-20T00:49:56Z

@bpasero squashed the commit and fixed merge errors. looks the the CI system may need to be updated?

bpasero · 2016-10-20T05:03:49Z

@wprater yes, the build errors are unrelated and should get resolved during the day today.

bpasero · 2016-10-20T05:55:17Z

@wprater can you merge with master to get build fixes?

Are you done, should I review again?

wprater · 2016-10-20T06:12:05Z

I'll rebase and push up when I get home. otherwise I'm done.

On Wed, Oct 19, 2016, 22:55 Benjamin Pasero notifications@github.com
wrote:

@wprater https://github.com/wprater can you merge with master to get
build fixes?

Are you done, should I review again?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#13925 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAo1FEq1THMTmVVsvxYNRo6N2A6voEMks5q1wJsgaJpZM4KZeug
.

wprater · 2016-10-20T07:49:30Z

rebased and pushed, @bpasero

new review

bpasero

More feedback

bpasero · 2016-10-20T14:06:40Z

src/vs/workbench/browser/quickopen.ts

+
+		let backgroundOpts;
+		if (mode === Mode.OPEN_IN_BACKGROUND) {
+			backgroundOpts = objects.assign({}, {


Why not just { pinned: true, preserveFocus: true } ??

needs cleanup, was trying a different approach earlier. thanks.

bpasero · 2016-10-20T14:08:02Z

src/vs/workbench/browser/quickopen.ts

-		if (mode === Mode.OPEN) {
+		const hideWidget = mode === Mode.OPEN;
+
+		let backgroundOpts;


I suggest to move this into the other if check where we actually care about backgroundOpts. It is not being used in the scope otherwise.

bpasero · 2016-10-20T14:10:24Z

src/vs/workbench/browser/quickopen.ts

 			let sideBySide = context.keymods.indexOf(KeyMod.CtrlCmd) >= 0;
+			let opts = this.getOptions();
+			if (backgroundOpts) {
+				opts = objects.assign(opts || {}, backgroundOpts);


You cannot use objects.assign with the opts here because opts can be a real instance of EditorOptions, it might not just be an options bag. Maybe the better fix is to see if this.getOptions() could always return IEditorOptions.

Any reason why we cannot merge the getOptions options with the (<IResourceInput>input).options? I know you previously mentioned it was not done.

If we can, it might be a nice approach to put the options logic in getOptions

public getOptions(mode: Mode): EditorOptions { let opts = {}; if (mode === Mode.OPEN_IN_BACKGROUND) { opts = { pinned: true, preserveFocus: true }; } return EditorOptions.create(opts); }

for now I'll just

public getOptions(): EditorOptions { return EditorOptions.create({}); }

bpasero · 2016-10-20T14:13:33Z

src/vs/workbench/browser/parts/quickopen/quickOpenController.ts

@@ -1115,6 +1115,9 @@ export class EditorHistoryEntry extends EditorQuickOpenEntry {

 			return true;
 		}
+		else if (mode === Mode.OPEN_IN_BACKGROUND) {


I think we could always just return super.run(mode, context) here because the parent class does not handle PREVIEW anyway.

sounds good. in my other PR, Im refactored all this logic out, anyhow.

bpasero · 2016-10-20T14:14:21Z

@wprater added more feedback, thanks 👍

wprater · 2016-10-20T20:19:25Z

np! check out the changes and you can merge, I squashed them.

not sure what is going on with the CI error tho?

- allows one to continually open files while keeping quick open widget focused - uses the right modifier key to open in background - can open in split view if using the cmd trigger key

bpasero · 2016-10-21T06:46:19Z

@wprater pretty cool, let me merge and do a little bit of cleanup based on discussion. Thanks!

msftclas added the cla-signed label Oct 18, 2016

jrieken assigned bpasero Oct 18, 2016

bpasero previously requested changes Oct 18, 2016

View reviewed changes

wprater commented Oct 18, 2016

View reviewed changes

wprater force-pushed the quickopen-in-background branch 2 times, most recently from 0a725b0 to afb3bd1 Compare October 20, 2016 00:41

bpasero closed this Oct 20, 2016

bpasero reopened this Oct 20, 2016

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

wprater force-pushed the quickopen-in-background branch from afb3bd1 to 1f4020b Compare October 20, 2016 07:48

bpasero requested changes Oct 20, 2016

View reviewed changes

wprater force-pushed the quickopen-in-background branch 2 times, most recently from 512ccda to e65a089 Compare October 20, 2016 23:24

quick open files in background

e8e3e35

- allows one to continually open files while keeping quick open widget focused - uses the right modifier key to open in background - can open in split view if using the cmd trigger key

wprater force-pushed the quickopen-in-background branch from e65a089 to e8e3e35 Compare October 20, 2016 23:29

bpasero merged commit 1d59084 into microsoft:master Oct 21, 2016

bpasero added a commit that referenced this pull request Oct 21, 2016

clean up from #13925

89fbdde

wprater deleted the quickopen-in-background branch October 21, 2016 07:58

wprater mentioned this pull request Oct 27, 2016

Open in background hotkey should only activate if cursor is at end of line #14350

Closed

bpasero added this to the October 2016 milestone Oct 28, 2016

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

quick open files in background #13925

quick open files in background #13925

wprater commented Oct 18, 2016

mention-bot commented Oct 18, 2016

msftclas commented Oct 18, 2016

jrieken commented Oct 18, 2016

wprater commented Oct 18, 2016

bpasero commented Oct 18, 2016

wprater commented Oct 18, 2016 •

edited

Loading

bpasero Oct 18, 2016

wprater Oct 18, 2016

bpasero Oct 18, 2016

wprater Oct 18, 2016

bpasero Oct 18, 2016

wprater Oct 18, 2016

bpasero Oct 18, 2016

wprater Oct 18, 2016

bpasero Oct 18, 2016

bpasero Oct 18, 2016

wprater Oct 18, 2016 •

edited

Loading

wprater Oct 18, 2016

wprater commented Oct 20, 2016

bpasero commented Oct 20, 2016

bpasero commented Oct 20, 2016

wprater commented Oct 20, 2016

wprater commented Oct 20, 2016

bpasero left a comment

bpasero Oct 20, 2016

wprater Oct 20, 2016

bpasero Oct 20, 2016

bpasero Oct 20, 2016

wprater Oct 20, 2016

bpasero Oct 20, 2016

wprater Oct 20, 2016

bpasero commented Oct 20, 2016

wprater commented Oct 20, 2016 •

edited

Loading

bpasero commented Oct 21, 2016

		} else {
		objects.assign((<IResourceInput>input).options, opts);

quick open files in background #13925

quick open files in background #13925

Conversation

wprater commented Oct 18, 2016

mention-bot commented Oct 18, 2016

msftclas commented Oct 18, 2016

jrieken commented Oct 18, 2016

wprater commented Oct 18, 2016

bpasero commented Oct 18, 2016

wprater commented Oct 18, 2016 • 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

wprater Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wprater commented Oct 20, 2016

bpasero commented Oct 20, 2016

bpasero commented Oct 20, 2016

wprater commented Oct 20, 2016

wprater commented Oct 20, 2016

bpasero left a 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

bpasero commented Oct 20, 2016

wprater commented Oct 20, 2016 • edited Loading

bpasero commented Oct 21, 2016

wprater commented Oct 18, 2016 •

edited

Loading

wprater Oct 18, 2016 •

edited

Loading

wprater commented Oct 20, 2016 •

edited

Loading