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

[Terminal] SomeKey Can't Work #5749

Closed
502647092 opened this issue Jul 19, 2019 · 26 comments
Closed

[Terminal] SomeKey Can't Work #5749

502647092 opened this issue Jul 19, 2019 · 26 comments
Labels
bug bugs found in the application keybindings issues related to keybindings OS/Windows issues related to the Windows OS terminal issues related to the terminal

Comments

@502647092
Copy link
Contributor

Description

latest version in terminal
Ctrl + C Can't Work on terminal

Reproduction Steps

OS and Theia version:
Diagnostics:

@502647092
Copy link
Contributor Author

test on gitpod.io browers example also can't work
@akosyakov

@502647092
Copy link
Contributor Author

502647092 commented Jul 19, 2019

i add config to keymap.json fix this problem

[
    {
        "command": "core.copy",
        "keybinding": "ctrlcmd+c",
        "context": "strictEditorTextFocus"
    }
]

maybe cause by #5326

@akosyakov akosyakov added bug bugs found in the application keybindings issues related to keybindings terminal issues related to the terminal labels Jul 19, 2019
@akosyakov
Copy link
Member

It works for me. @kittaakos @vince-fugnitto Could you try as well?

@akosyakov
Copy link
Member

@502647092 Which os and browser versions?

@502647092
Copy link
Contributor Author

@502647092 Which os and browser versions?

windows10 chrome 75.0

@kittaakos
Copy link
Contributor

Works for me on Windows in Chrome:

 git clean -ffxqd && git reset --hard && git rev-parse --short HEAD && yarn && yarn --cwd ./examples/browser/ start
Checking out files: 100% (1731/1731), done.
HEAD is now at 3587c237c Add 'theia-widget-noInfo' css class to fix message inconsistencies
3587c237c

screencast 2019-07-19 12-45-10

@502647092
Copy link
Contributor Author

@kittaakos Not Copy
i want to close process

@kittaakos
Copy link
Contributor

@kittaakos Not Copy
i want to close process

Well, yeah both cannot work. I can confirm, I could not SIGTERM the process from the terminal with Ctrl + C on Windows.

@akosyakov
Copy link
Member

So it is not a regression?

@kittaakos
Copy link
Contributor

So it is not a regression?

Yeah, seems to be a bug.

I tried if disabling the copy in the terminal helps as quick workaround, but no.

https://github.com/theia-ide/theia/blob/3587c237cacc638dc54ec19dc3eaa755c2e69fb5/packages/terminal/src/browser/terminal-widget-impl.ts#L499-L500

Screen Shot 2019-07-19 at 17 29 56

root ERROR Error: No option with key "enableCopy"
    at Terminal.push.../../node_modules/xterm/lib/Terminal.js.Terminal.setOption (http://localhost:3000/34.bundle.js:507
6:19)
    at Terminal.push.../../node_modules/xterm/lib/public/Terminal.js.Terminal.setOption (http://localhost:3000/34.bundle
.js:7906:20)
    at http://localhost:3000/40.bundle.js:363:28
    at http://localhost:3000/bundle.js:109260:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:109275:
39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:109363:29)
    at http://localhost:3000/bundle.js:93655:40
    at http://localhost:3000/bundle.js:109260:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:109275:
39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:109363:29)
root ERROR Error: No option with key "enablePaste"
    at Terminal.push.../../node_modules/xterm/lib/Terminal.js.Terminal.setOption (http://localhost:3000/34.bundle.js:507
6:19)
    at Terminal.push.../../node_modules/xterm/lib/public/Terminal.js.Terminal.setOption (http://localhost:3000/34.bundle
.js:7906:20)
    at http://localhost:3000/40.bundle.js:363:28
    at http://localhost:3000/bundle.js:109260:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:109275:
39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:109363:29)
    at http://localhost:3000/bundle.js:93655:40
    at http://localhost:3000/bundle.js:109260:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:109275:
39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:109363:29)

@kittaakos
Copy link
Contributor

But my exception can be an unrelated issue.

@502647092
Copy link
Contributor Author

maybe is context problem?
i add config can fix it

@roflcoopter
Copy link

Experiencing the same.
Before i could use Ctrl+C to close a tail -f and if text was highlighted in the terminal Ctrl+C would copy it.

Now Ctrl+C to terminate the command doesn't work, same setup. Windows 10 Chrome 75

@akosyakov akosyakov added the OS/Windows issues related to the Windows OS label Jul 20, 2019
@AndrienkoAleksandr
Copy link
Contributor

I think we should copy by Ctrl+C in case if terminal has selection

xterm.js selection docs

otherwise we shouldn't handle this hotkey at all.

@kittaakos
Copy link
Contributor

if terminal has selection

https://github.com/theia-ide/theia/blob/3587c237cacc638dc54ec19dc3eaa755c2e69fb5/packages/terminal/src/browser/terminal-widget-impl.ts#L501

@kittaakos
Copy link
Contributor

I have switched back to da653c2, which is one commit before the #5326.

Screen Shot 2019-07-22 at 10 22 31

I can confirm, SIGTERM with ctrl+c works on Windows from the terminal:

screencast 2019-07-22 10-58-28

I am going to revert #5326.

kittaakos added a commit that referenced this issue Jul 22, 2019
This reverts commit 1f9472e.

Reverts #5326.
Closes #5749.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@JPinkney
Copy link
Contributor

My only guess would be that somewhere in https://github.com/theia-ide/theia/blob/master/packages/core/src/browser/keybinding.ts#L557 its trying to apply ctrl+c for copy instead. I don't have a windows machine but I'll try to get one so I can find out whats causing the issue

@akosyakov
Copy link
Member

Yes, it looks like there are 2 bindings matching and order before was different. A agree with #5749 (comment) one this bindings should have more specific context or when closure to disambiguate.

@JPinkney we revert for now, please open a new PR

kittaakos pushed a commit that referenced this issue Jul 23, 2019
This reverts commit 1f9472e.

Reverts #5326.
Closes #5749.

Signed-off-by: Akos Kitta <kittaakos@typefox.io>
@akosyakov
Copy link
Member

Reopen it since it should be taken into the consideration when someone reverts dd51244

@akosyakov akosyakov reopened this Jul 23, 2019
@eduprovide

This comment has been minimized.

@akosyakov
Copy link
Member

We should also revert #6170 when we align with VS Code by reverting dd51244.

@tsmaeder
Copy link
Contributor

I'm observing this problem now: I was able to verify that it still worked in commit 3f28503, but is broken in 362e8f1

@tsmaeder
Copy link
Contributor

Was able to trace this back to this commit: 62c9a83

@akosyakov
Copy link
Member

@tsmaeder I think it is a new regression. Could you file a new issue please with reproducible steps? I will have a look at it tomorrow.

@502647092
Copy link
Contributor Author

@akosyakov recent version cause Tmux can't received prefix
Ctrl + A => d
github

@akosyakov
Copy link
Member

resolved by #7839

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application keybindings issues related to keybindings OS/Windows issues related to the Windows OS terminal issues related to the terminal
Projects
None yet
Development

No branches or pull requests

8 participants