Exception for using electron API for OS events.

[seivan]

There is no way to get access to the OS level Swipe events outside of Electron.
Meaning if the user wants to listen to it, the only option is to monkey patch the VSCode boot and inject listeners.

I am hoping there can be an exception for this event so this PR can be accepted.

All it really does is listen to the event and forward the action, nothing more.
There isn't much that can break, so I don't quite understand the resistance since the Electron API is already used for every other event that comes in.

Edit: Made an extension out of it which is a good compromise.

[bpasero]

There isn't much that can break, so I don't quite understand the resistance since the Electron API is already used for every other event that comes in.

I disagree. As you can see from electron/electron#19131, the swipe event was broken in Electron 3, 4, 5, and earlier versions of 6.

I am open to bringing this forward to browser vendors to support this via HTML officially and then drive this feature from there. This also ensures that our web-only users benefit from such a feature.

[seivan]

@bpasero You misunderstood: I meant breaking in terms of the code implemented on this end, not on Electrons end.

It's a pretty shitty situation with Electron breaking, but it's fixed now, so I don't see why it shouldn't be used, I mean where do you draw the line on what OS level events to listen to and not?

The way the PR is implemented, was that it was default off and could be toggled for different kinds of navigation - I recall some of the confusion was that people expected more of a browser based transition vs the one I'm familiar with from Xcode.

Edit: I also added app-command for back/forward commands outside of Swipes - that should(?) work in a browser if that's part of the hesitance.

[bpasero]

@seivan I think I did understand, I would like to reduce chances that VSCode breaks with future Electron updates by reducing the number of APIs we actually leverage. Where possible, I want us to switch to HTML APIs and away from Electron APIs. This has 2 advantages:

• we are independent from Electron breakage (yes browsers can break, but rather unlikely)
• we support new features in both desktop and web

[seivan]

@bpasero My take is, if there is already Electron listeners for other unique OS level events, I don't see how this makes a difference?

I understand the path is to eventually have a fully browser compliant implementation but since that's not possible today with even greater hurdles to other features implemented with Electron, working with the OS level gesture offers a better UX than implementing it over the dom, not to mention more light weight.

This is the only thing attached to Electron

this._win.addListener(command, (_event: Event, direction: string) => {
 			switch (direction) {
 				case back: this.sendWhenReady('vscode:runAction', { id: actions.back });
 				case forward: this.sendWhenReady('vscode:runAction', { id: actions.forward });
 			}
 		});

Everything else is related to config and the vscode:runAction does the heavy lifting.

Any other swipe gesture recognizer will most certainly be heavier and not certain it will perform as well (today) as the OS level. I understand you want to wait until it's implemented in all the browsers.

I don't think it's an unreasonable stance you're taking here, I recall Apple had implemented a lower level Gesture Recognizer API to recreate it but it's going to be a while until things are solidified across vendors.

Look, I am not going to argue further if you guys have set your mind on freezing any new Electron API usage - I just wanted to present my case, and I've gone as far as I can.

Feel free to close this ticket if we're not moving ahead with the current implementation available.

[bpasero]

@seivan on the Electron front I can be convinced to tolerate new API if you can come up with a unit test (on https://github.com/atom/electron) that clearly fails when the feature does not work anymore. That is the absolute minimum that we need to move forward. I think that makes sense?

To make things a little bit more complicated, this unit test needs to run on the supported macOS versions: 10.10, 10.11, 10.12, 10.13, 10.14 and 10.15.

Just to give an example, native Sierra macOS native tabs recently broke with the Electron update out of nowhere for macOS <= 10.12 (#76537).

[seivan]

@bpasero Alright, now we're talking! But unfortunately I can't commit to guarantee a solid test for all of those.
I could rely on CI matrices for a feedback loop, but it would be too time consuming.
That being said, I like the approach and I think it's more than fair, so thanks for that. 👍

If it wasn't clear I'm just writing this feature for myself to be honest and I'm on Mojave(10.14.6) - will eventually move to Catalina if just those two I could investigate how get there.

Anyway thanks, but I've managed to get a monkey patch working so I'm set. It's no longer the end of the world!

Edit: Hmm, I'm thinking if I make an extension out of the monkey patch and over time write tests directly on a VSCode fork with just that extension, assuming there hasn't been a pure browser implementation by then, that could eventually be ported over once all matrices are done.

[bpasero]

I think we can close this issue as we identified a path forward (have a unit test in Electron). I will update our PR guidelines to make this clear.