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

Fix HOME, PageUp, PageDown shortcuts for terminal. #7305

Merged
merged 1 commit into from Mar 10, 2020

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Mar 10, 2020

Referenced issue:

#7304

What it does

Fix HOME, PageUp, PageDown shortcuts for terminal.

How to test

Use 'Shift+Home' shortcut to scroll terminal to top
Use 'Shift+PageUp' to scroll one terminal page
Use 'Shfit+PageDown' to scroll terminal bottom

Review checklist

Reminder for reviewers

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@akosyakov akosyakov added the terminal issues related to the terminal label Mar 10, 2020
@akosyakov
Copy link
Member

akosyakov commented Mar 10, 2020

@AndrienkoAleksandr Do you know why just home does not work anymore? It used to work before upgrade.

@akosyakov akosyakov added the keybindings issues related to keybindings label Mar 10, 2020
@AndrienkoAleksandr
Copy link
Contributor Author

@AndrienkoAleksandr Do you know why just home does not work anymore? It used to work before upgrade.

nope, I can take look. But with current pr, Home works - move cursor to the begin of the line.

@jankeromnes
Copy link
Member

jankeromnes commented Mar 10, 2020

Just tested this PR, it works well! 🎉 Many thanks, I'm able to use HOME in order to jump to the beginning of the line again.

Also, I notice that Option + LeftArrow and Option + RightArrow also work now (to jump from word to word on a macbook). This is fantastic. I'm not sure if that's thanks to your PR or not, but I remember training myself not to use it because it was broken. It's extremely cool if that's working again too now. 💯

TL;DR I confirm this PR fixes #7304.

@akosyakov
Copy link
Member

akosyakov commented Mar 10, 2020

@jankeromnes please approve if it is fine, I'm a bit concerned that we have to change keybindings and don't know why old keybindings don't work anymore, but if new keybindings are better then probably it is not important.

@jankeromnes
Copy link
Member

jankeromnes commented Mar 10, 2020

I'm a bit concerned that we have to change keybindings and don't know why old keybindings don't work anymore

It's because these particular keybindings were introduced recently (13-20 days ago), in #7179 and in particular 8935269#diff-b30aa6cfa31ef8d5a02459c5454b81b3R372-R386.

So this is a follow-up to #7179 that prefixes 3 newly introduced keybindings with Shift (fine by me).

Now that I think of it, I think I may have also noticed less being broken, because PageUp was doing something weird. This PR probably fixes that issue as well.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

@AndrienkoAleksandr thanks for fixing
@jankeromnes thanks for testing and explaining what was wrong!

@jankeromnes
Copy link
Member

Thanks @akosyakov for approving and merging! And indeed thanks @AndrienkoAleksandr for fixing this so quickly.

I have two remaining questions, if you have time:

  • Is there a difference between keybinding: 'shift-home' and keybinding: 'shift+home'? (I noticed that keybindings above use + instead of -.)

  • What is the purpose of the new shift-pageUp if the pre-existing pageUp already works well in the Terminal?

@AndrienkoAleksandr
Copy link
Contributor Author

if the pre-existing pageUp already works well in the Terminal

Actually it works not always - depends on shell and program opened in the shell. That's why we used program scroll embedded to the xterm.js. For example for my working laptop and my shell - it doesn't work(Fedora 31, shell - bash).

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

Successfully merging this pull request may close these issues.

None yet

3 participants