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

Switching pane-maximize will add an additional line break in the new window #8071

Closed
dhslegen opened this issue Mar 13, 2023 · 10 comments · Fixed by #8471
Closed

Switching pane-maximize will add an additional line break in the new window #8071

dhslegen opened this issue Mar 13, 2023 · 10 comments · Fixed by #8471

Comments

@dhslegen
Copy link

Describe the problem:
Switching pane-maximize will add an additional line break in the new window, which can be solved by changing a shortcut key, but I'm still used to the combination of cmd option enter.

image

To Reproduce:
split window>cmd option enter * n

@echo304
Copy link

echo304 commented May 15, 2023

@Eugeny Can I take this issue?

@Eugeny
Copy link
Owner

Eugeny commented May 15, 2023

of course!

@echo304
Copy link

echo304 commented May 16, 2023

@Eugeny Can you provide some advice that where all the key strokes are handled?
I suspected that predefined hotkey(cmd + option + enter) somehow affect to the terminal command line.
Reason why I think is that if I change hotkey to other key combination without 'enter' it does not add line break.

@Eugeny
Copy link
Owner

Eugeny commented May 16, 2023

All hotkeys are handled in https://github.com/Eugeny/tabby/blob/master/tabby-core/src/services/hotkeys.service.ts , the likely solution is to preventDefault the keyboard event once a hotkey is recognized

GitHub
A terminal for a more modern age. Contribute to Eugeny/tabby development by creating an account on GitHub.

@echo304
Copy link

echo304 commented May 18, 2023

I tried to figure out where those keyboard event is propagated in hotkeys.service.ts but it didn't work well.
I guess there is leaking(propagated) keyboard event of "Enter" somewhere... 🕵️ or somewhere around NgZone.
So, I'm going to dig where actual input is added to the UI.

sendInput (data: string|Buffer): void {
if (!(data instanceof Buffer)) {
data = Buffer.from(data, 'utf-8')
}
this.session?.feedFromTerminal(data)
if (this.config.store.terminal.scrollOnInput) {
this.frontend?.scrollToBottom()
}
}

Is this right place to start? 🤔
@Eugeny

@Eugeny
Copy link
Owner

Eugeny commented May 18, 2023

Not exactly, xterm is listening to keyboard events directly, so terminal input doesn't to through tabby code, but I'm intercepting all keys in HotkeyService first, where, if a key matches a known shortcut, it should be cancelled and never make it's way into xterm

@echo304
Copy link

echo304 commented May 18, 2023

Got it. 👍

@Issues-translate-bot
Copy link
Collaborator

The translator bot has detected that this issue body's language is not English, and has translated it automatically.


Got it. 👍

@echo304
Copy link

echo304 commented May 21, 2023

If I commented out these lines, this bug is removed but simply removing these lines is definitely not the solution.
I briefly understand why they are necessary thanks to the comment // macOS will swallow non-modified keyups if Cmd is held down. Can you explain the more detailed background of this code snippet?

if (process.platform === 'darwin' && eventData.metaKey && eventName === 'keydown' && !['Ctrl', 'Shift', altKeyName, metaKeyName].includes(keyName)) {
// macOS will swallow non-modified keyups if Cmd is held down
this.pushKeyEvent('keyup', nativeEvent)
}

@Eugeny
Copy link
Owner

Eugeny commented May 21, 2023

Taking these lines out won't do anything for other operating systems unfortunately, as macOS is the only one with this problem. On other OSes, keyup will still fire by itself as expected. The problem is that HotkeyService doesn't seem to intercept/cancel keyup that corresponds to a recognized shortcut, and it's getting passed down to the terminal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants