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

respect keybinding scope and registration order during evaluation #7839

Merged
merged 1 commit into from
May 20, 2020

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented May 19, 2020

What it does

How to test

Review checklist

Reminder for reviewers

@akosyakov
Copy link
Member Author

@tsmaeder @RomanNikitenko @502647092 Could you check please whether it resolves issues mentioned in #5749, #7833 and #7836?

I'm testing too, but I'm on Mac.

@akosyakov akosyakov added keybindings issues related to keybindings monaco issues related to monaco vscode issues related to VSCode compatibility labels May 19, 2020
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I tested these changes on Fedora 31
The PR fixes use cases which I described in #7833

fix_ctrl_c

@tsmaeder
Copy link
Contributor

Ctrl-c seems to work on Windows FF and Chromium.

@tsmaeder
Copy link
Contributor

I've found a different problem, though: every once in a while (seems more frequent on Firefox), the following task:

{
            "type": "shell",
            "command": "timeout /T 10",
            "label": "wait",
            "problemMatcher":"$eslint-compact"
        }

Seems to terminate with exit code 0 immediately instead of waiting 10 seconds.
@akosyakov is your branch based on latest master?

@akosyakov
Copy link
Member Author

@akosyakov is your branch based on latest master?

yes, i don't think it is related to keybindings handling

@akosyakov
Copy link
Member Author

akosyakov commented May 19, 2020

I'm:

  • adding more regression tests for the monaco extension
  • adding more regression tests for the terminal extensions
  • trying to resurrect some fixes from [keybindings] Aligned keybindings with VSCode #5326 from @JPinkney (code was changed in the meantime unfortunately)
    • registered collided keybindings with higher priority, don't fail as before
      • there is not really a way to detect whether keybindings are collided without evaluating them in some context
    • filter out removed keybindings during evaluation

@akosyakov akosyakov force-pushed the akosyakov/ctrl-c-terminate-not-7836 branch from 9afdbd3 to c133987 Compare May 19, 2020 10:03
@tsmaeder
Copy link
Contributor

I've built master after deleting all node_modules folders, and I can't observer the "early exit"-behaviour anymore. Not sure what did it, though.

@akosyakov
Copy link
Member Author

@502647092 I would love to have your feedback whether it resolves issues in #5749?

@akosyakov akosyakov force-pushed the akosyakov/ctrl-c-terminate-not-7836 branch 3 times, most recently from ae9a884 to 25db922 Compare May 19, 2020 13:32
…luation

Also match only first keybinding on keydown, don't traverse over all keybindings.

Signed-off-by: Anton Kosyakov <[email protected]>
@akosyakov akosyakov force-pushed the akosyakov/ctrl-c-terminate-not-7836 branch from 25db922 to 8888a8b Compare May 19, 2020 13:57
@akosyakov
Copy link
Member Author

@RomanNikitenko could you double test it please? I have changed a lot since you looked at it. Thank you 🙏

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented May 19, 2020

@akosyakov
Tested on Fedora terminate process in terminal and terminate a task after your update - it works well for me!
terminate_test

@akosyakov
Copy link
Member Author

akosyakov commented May 19, 2020

@RomanNikitenko ok, thanks, I will merge tomorrow morning if there won't be any other feedback

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented May 19, 2020

Also I tried for emacs:

  • select all - ctrl-x h
  • quick open for a file - ctrl-x b

emacs

@502647092
Copy link
Contributor

@akosyakov It looks like it can work

@akosyakov akosyakov merged commit eeaf506 into master May 20, 2020
@akosyakov akosyakov deleted the akosyakov/ctrl-c-terminate-not-7836 branch May 20, 2020 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keybindings issues related to keybindings monaco issues related to monaco vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ctrl-c (terminate) not working in Terminals
4 participants