Fixes #4803: Adding support for swipe gestures on macOS. by seivan · Pull Request #23663 · microsoft/vscode

seivan · 2017-03-30T11:40:49Z

Disabled by default, enabled with:

"workbench.editor.swipeToNavigate": true

mention-bot · 2017-03-30T11:40:51Z

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

msftclas · 2017-03-30T11:40:54Z

@seivan,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

bpasero · 2017-03-30T22:59:54Z

@seivan should this work out of the box or do I need to configure my trackpad specifically?

seivan · 2017-03-30T23:02:01Z

@bpasero It should work out of the box (with default macOS settings) but I'm not sure since I've changed my settings to this:

bpasero · 2017-03-30T23:02:29Z

It seems to me this does not work when trackpad is configured like this:

seivan · 2017-03-30T23:03:11Z

@bpasero My take is it's an Electron issue if anything, since we're just listening to events here.

seivan · 2017-03-30T23:04:42Z

Event: 'swipe' macOS

Returns:

event Event
direction String

Emitted on 3-finger swipe. Possible directions are up, right, down, left.

Emphasis mine.

bpasero · 2017-03-30T23:06:09Z

Ok the only way I get this to work is with the following config:

Does somebody know if that is the default on macOS?

bpasero · 2017-03-30T23:08:19Z

src/vs/code/electron-main/window.ts

 		});

+		// Swipe command support (macOS)
+		this._win.on('swipe', (e, cmd) => {


@seivan can we reuse this code with the on('app-command') handler? Seems duplicate.

@bpasero Read my mind. I thought about it but I wasn't sure you guys would want it. We could take an object of events and left/right options to compare to.

[{event: "swipe", leftAction :"...", rightAction:"..."}, ...]
or just call a function twice with the params. Up to you.

Is that OK with you?

@seivan extracting the code to a function and calling it works for me best

seivan · 2017-03-30T23:08:39Z

@bpasero This seems like intended behaviour as Electron does not switch between full-screen apps but between pages and it needs three fingers as two fingers is scrolling.

bpasero · 2017-03-30T23:10:56Z

@seivan makes sense, I asked on the related issue for more feedback from users. Otherwise your PR seems like the way to go (I added little bit of feedback).

bpasero · 2017-04-06T05:47:03Z

@seivan ping

seivan · 2017-04-06T05:50:14Z

@bpasero I thought we were still waiting for feedback. My bad. I'll get it on later tonight.

bpasero · 2017-04-06T06:30:49Z

Thanks!

…e method that defines back and forward commands.

bpasero · 2017-04-07T05:02:28Z

@seivan the more I think about it the more I feel this should be controlled via a new setting so that users can opt-in to this feature and report back how it works. I suggest something like workbench.swipeToNavgiate (boolean) and wire it up. You can see for other settings like window.titleBarStyle how this is wired up.

seivan · 2017-04-07T05:47:54Z

@bpasero Yeah that makes sense. I'll give it a go tomorrow.

…ettings. Importing `IWorkbenchEditorConfiguration` to `window.ts` to listen settings before registering macOS Swipes. Refactoring `configurationRegistry` to setup properties first so it can use conditionals for macOS settings.

seivan · 2017-04-07T07:30:48Z

@bpasero I didn't have time to check, but do you 're-run' registerListeners() on settings change?
Or do you need to trigger a function on setting changes?

If you do not re-run registerListeners() then I need to listen to setting changes to remove the swipe-listener, not sure where to find those.

bpasero · 2017-04-07T08:37:45Z

@seivan yes you should listen to the settings change and adjust accordingly, see https://github.com/Microsoft/vscode/blob/master/src/vs/code/electron-main/window.ts#L246 for example how we do it there.

seivan · 2017-04-12T09:24:02Z

@bpasero Sorry, I don't quite follow, could you show which specific callback to listen to? Your link seems outdated now.

Is it

this.onConfigurationUpdated(this.configurationService.getConfiguration<IConfiguration>());

Do I need one for the workBench?

bpasero · 2017-04-12T11:56:17Z

@seivan yes that is the right listener to use, it should be available on the main side to listen for config changes (maybe I did not understand your question properly?)

seivan · 2017-04-12T12:07:37Z

@bpasero Just to clear the ambiguity, I am just not sure where I'm supposed to to use this.onConfigurationUpdated { .... } and check for enable/disable and set the listener or remove it?

bpasero · 2017-04-12T12:28:10Z

@seivan I would say you can maybe remove the swipe listener (or add it) based on the configuration change event, iff the value of this setting changes. So I would say that listening on this event around where you register the swipe listener is the right place.

seivan · 2017-04-16T15:33:33Z

@bpasero Yeah, that's what I am asking; what's the method that triggers on configuration change? Will it also trigger on boot? So I can just keep it in one place?

…ng swipe listeners. Also removed config argument from `getMenuBarVisibility()` since it wasn't being used.

seivan · 2017-04-16T15:48:44Z

@bpasero Alright, so I think I figured it out. Take a look, thanks!

bpasero

Getting closer, some minor feedback included.

bpasero · 2017-04-18T05:51:48Z

src/vs/code/electron-main/window.ts

+			} else if (cmd === forward) {
+				this.send('vscode:runAction', 'workbench.action.navigateForward');
+			} else {
+				console.warn('[electron command]: did not handle: ', command);


Should not happen, please remove.

The cmd is a string. It could technically happen? You want the warning removed?

You could guard against this by using a string variable, e.g. 'foo' | 'bar' so that noone can pass anything else in.

@bpasero We can't predict all the available commands for every event related to navigation that exists. You would have to maintain a list somewhere to keep them up to date.
E.g for swipe it's left and right but on app-command it's: browser-backward and browser-forward

But we are the only ones calling this method, no? Why do we expect to print a warning? This is not API anyone else is using?

@bpasero Imagining future navigation gestures that have unhandled cmds. It's up to you. If you want it gone, I'll remove it.

Yeah, lets remove it for now, thanks 👍

bpasero · 2017-04-18T05:52:20Z

src/vs/code/electron-main/window.ts

+
+		// Swipe command support (macOS)
+		const workbenchConfig = this.configurationService.getConfiguration<IWorkbenchEditorConfiguration>();
+		if (workbenchConfig && workbenchConfig.workbench && workbenchConfig.workbench.editor.swipeBetweenOpenFiles) {


Careful here, workbenchConfig.workbench.editor can be null so we need another null check.

Ah good point. I was thinking maybe they should be defined as optionals as I actually thought of doing that refactoring, but maybe that should be a different PR?

I feel having to do those checks without the compiler enforcing it is wrong per se.

But I'll get on this later tonight. Thanks for catching it!

Edit: Actually, editor can't be null, I remember testing it. workBench could be null and did throw, but editor wasn't. I am not exactly sure how this works though, so maybe you have better insights here.

All of it can be null as soon as a user configures something like this in settings:

{ "editor": null }

Ah I understand!
How do you feel about making this more inline with TS and using optionals? Should prevent future issues like this.

@bpasero I can take that in a different PR if it would be of interest, basically the structure now could technically be changed to a class with optional attributes. It's just a regular object for now(?). I'll leave it and do the checks for now.

We cannot change this, it is just a plain old object after all.

bpasero · 2017-04-18T05:53:33Z

src/vs/workbench/electron-browser/main.contribution.ts

+};
+
+if (isMacintosh) {
+	workbenchProperties['workbench.editor.swipeBetweenOpenFiles'] = {


I am not a big fan of the naming swipeBetweenOpenFiles, because in reality you can swipe left and right between any editor that is opened, not just files. How about naming this workbench.editor.swipeToNavigate?

@bpasero You're right, I'm not happy with the name either, changed it twice, lol.
Sure, that's a good call.

…ngs for unhandled commands for listener.

seivan · 2017-04-18T19:34:44Z

@bpasero Take another look.

bpasero · 2017-04-19T06:42:21Z

Thanks, looks good now!

Fixes #4803: Adding support for swipe gestures on macOS.

d155179

msftclas added the cla-signed label Mar 30, 2017

running formatter

4a04814

aeschli assigned bpasero Mar 30, 2017

bpasero requested changes Mar 30, 2017

View reviewed changes

bpasero mentioned this pull request Mar 30, 2017

Add support for swipe gestures on Mac #4803

Closed

Refactor duplicate listeners for 'app-command' & 'swipe' into a singl…

8c160d4

…e method that defines back and forward commands.

Listening to onConfigurationUpdated() callback for adding or removi…

be819eb

…ng swipe listeners. Also removed config argument from `getMenuBarVisibility()` since it wasn't being used.

seivan added 3 commits April 16, 2017 19:17

Ensure workbench exists before accessing editor.

4c8f903

Rename swipeToNavigateTabs to swipeBetweenOpenFiles

e2d6637

Use swipeBetweenOpenFiles

09ae872

bpasero requested changes Apr 18, 2017

View reviewed changes

seivan added 2 commits April 18, 2017 20:25

Renamed swipeBetweenOpenFiles to swipeToNavigate and remove warni…

1c67afc

…ngs for unhandled commands for listener.

Removing experimental optionals for IWorkbenchEditorConfiguration

40890c1

bpasero merged commit bd84e6d into microsoft:master Apr 19, 2017

bpasero added this to the April 2017 milestone Apr 19, 2017

seivan mentioned this pull request Oct 14, 2019

Adding three finger swipe with various options. #82528

Closed

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

Conversation

seivan commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 30, 2017

Uh oh!

msftclas commented Mar 30, 2017

Uh oh!

bpasero commented Mar 30, 2017

Uh oh!

seivan commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpasero commented Mar 30, 2017

Uh oh!

seivan commented Mar 30, 2017

Uh oh!

seivan commented Mar 30, 2017

Event: 'swipe' macOS

Uh oh!

bpasero commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seivan Mar 30, 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!

seivan commented Mar 30, 2017

Uh oh!

bpasero commented Mar 30, 2017

Uh oh!

bpasero commented Apr 6, 2017

Uh oh!

seivan commented Apr 6, 2017

Uh oh!

bpasero commented Apr 6, 2017

Uh oh!

bpasero commented Apr 7, 2017

Uh oh!

seivan commented Apr 7, 2017

Uh oh!

seivan commented Apr 7, 2017

Uh oh!

bpasero commented Apr 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

bpasero commented Apr 12, 2017

Uh oh!

seivan commented Apr 12, 2017

Uh oh!

bpasero commented Apr 12, 2017

Uh oh!

seivan commented Apr 16, 2017

Uh oh!

seivan commented Apr 16, 2017

Uh oh!

bpasero left a comment

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!

bpasero Apr 18, 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!

Choose a reason for hiding this comment

seivan commented Mar 30, 2017 •

edited

Loading

seivan commented Mar 30, 2017 •

edited

Loading

bpasero commented Mar 30, 2017 •

edited

Loading

seivan Mar 30, 2017 •

edited

Loading

seivan commented Apr 12, 2017 •

edited

Loading

bpasero Apr 18, 2017 •

edited

Loading

seivan Apr 18, 2017 •

edited

Loading

bpasero Apr 18, 2017 •

edited

Loading