Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding three finger swipe with various options. #82528

Closed
wants to merge 2 commits into from
Closed

Adding three finger swipe with various options. #82528

wants to merge 2 commits into from

Conversation

seivan
Copy link
Contributor

@seivan seivan commented Oct 14, 2019

Following options have been added:

  • 'actions' -> Swipes will navigate recent action history.
  • 'tabs' -> Swipes will navigate currently opened tabs.
  • 'recent-tabs' -> Swipes will navigate recent tab history.
  • 'grouped-tabs' -> Swipes will navigate currently opened tabs in selected group.

Fix: #4803 (again)
Based on changes from: e368b34 from @bpasero & and my own PR #23663

Also fixes the confusion on what a swipe is supposed to do.
It seems most people just want or expect it to move between tabs, while I like it to function in similar fashion as Xcode.
These added options allows user to toggle their preferred setting. But it only works with a three (3) finger Swipe, as that's what the exposed Electron API uses.

Discussions:
#40526
#25447
#25325
#25449

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

@@ -319,6 +321,48 @@ export class CodeWindow extends Disposable implements ICodeWindow {
});
}


private registerSwipeNavigationListener(command: any, back: 'left' | 'browser-backward', forward: 'right' | 'browser-forward', config: 'actions' | 'recent-tabs' | 'tabs' | 'grouped-tabs'): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't use 'swipe' | 'app-command' type union here, since the definitions from Electron didn't recognize them for some reason. Open for suggestions on how to mitigate that. Adding string didn't work either.

Following options have been added: `'actions', 'tabs', 'recent-tabs', 'grouped-tabs'`.
Fix: #4803
Based on changes from:
e368b34 & #23663
this._win.removeAllListeners('app-command');
if (config && config.workbench && config.workbench.editor) {
let swipeConfig = config.workbench.editor.swipeToNavigate;
if (swipeConfig && swipeConfig !== 'off') {
Copy link
Contributor Author

@seivan seivan Oct 14, 2019

Choose a reason for hiding this comment

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

I am not 100% sure, but it seems the integration tests run with certain values within the config as null.
I'm guessing that workBench is null in this particular test, so calling editor on it failed here.

So with that in mind I assume the type definitions in TS are invalid, since they don't indicate that they're optionals.
I'm checking every single value here just to see what the CI is saying.

@bpasero
Copy link
Member

bpasero commented Oct 15, 2019

Thanks, but I am not accepting pull requests that leverage Electron APIs. Please see https://github.com/microsoft/vscode/wiki/How-to-Contribute#pull-requests and thanks for understanding.

Avoid changes that require specific Electron APIs, instead try to implement your change leveraging official Web APIs.

@bpasero bpasero closed this Oct 15, 2019
@seivan
Copy link
Contributor Author

seivan commented Oct 15, 2019

@bpasero I am sorry to hear, but I kinda rely heavily on this sort of UX for my workflow, coming from Xcode it's very useful to being able to have a single window and swiping quickly through history - even between tabs and past opened files.

Is there anyway we could make an exception and isolating it for macOS only somehow?
I investigated if this was possible as an extension, and it isn't.
Is there any other approach of injecting this feature in without relying on keeping a fork up to date?

@bpasero
Copy link
Member

bpasero commented Oct 19, 2019

@seivan I think I already mentioned our options here:

  • we add a test for Electron to prevent breakage in the future
  • we find a HTML spec equivalent that does not require Electron APIs

@seivan
Copy link
Contributor Author

seivan commented Oct 19, 2019

@bpasero Yep, that's a four day old comment that was edited before this discussion #82588

@bpasero
Copy link
Member

bpasero commented Oct 19, 2019

@seivan oh sorry, I think GitHub is really in trouble with notifications, I am getting old notifications showing up as new notifications!

@becca you might want to be aware of this. Happened twice to me already.

@seivan
Copy link
Contributor Author

seivan commented Oct 19, 2019

@bpasero I think it's me that edited a type in your username. It's my fault, I'm sorry.

@bpasero
Copy link
Member

bpasero commented Oct 20, 2019

Ah easy, no worries. Maybe that happened in another case as well.

@becca
Copy link

becca commented Oct 21, 2019

@bpasero can you please provide more information? here or via email works. are these notifications for commits/comments/reviews and how are these notifications being delivered (web or email)?

thanks for the heads up!

@bpasero
Copy link
Member

bpasero commented Oct 21, 2019

@becca I only consume notifications via web, but if editing it causes it to be shown again, then it would explain why this happened.

@becca
Copy link

becca commented Oct 21, 2019

depending on the edits being made that could be what's happening. is there a specific event you're referring to here that i should 👀?

@bpasero
Copy link
Member

bpasero commented Oct 21, 2019

I will keep an eye out if this happens again and let you know, thanks for reaching out here.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for swipe gestures on Mac
3 participants