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.

Sign up for GitHub

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

Jump to bottom

Add terminal scroll commands. #7179

Merged
merged 1 commit into from
Feb 26, 2020
Merged

Add terminal scroll commands. #7179

merged 1 commit into from
Feb 26, 2020

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Feb 19, 2020 •
edited
Loading

Add terminal scroll commands.

What it does

Add terminal scroll commands:

  • scroll line up by shortcut: Ctrl + Shift + ArrowUp
  • scroll line down by shortcut: Ctrl + Shift + ArrowDown
  • scroll to top of the terminal(beginning line) by shortcut: Home
  • scroll page up by shortcut PageUp
  • scroll page down by shortcut PageDown

How to test

  1. Open terminal, types some content to make scrollbar appear.
  2. Use shortcuts described above to check scroll commands.

Review checklist

Reminder for reviewers

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

@vince-fugnitto vince-fugnitto added accessibility issues related to accessbilitiy keybindings issues related to keybindings terminal issues related to the terminal labels Feb 19, 2020
vince-fugnitto
vince-fugnitto reviewed Feb 19, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I verified and it works correctly, the keybindings allow me to easily navigate the terminal when there is a lot of content (scrollbar exists).

packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-frontend-contribution.ts Outdated Show resolved Hide resolved
packages/terminal/src/browser/terminal-frontend-contribution.ts
(this.shell.activeWidget as TerminalWidget).scrollLineDown();
}
});
commands.registerCommand(TerminalCommands.SCROLL_TO_TOP, {
Copy link
Member

Choose a reason for hiding this comment

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

@AndrienkoAleksandr are we missing the keybinding for the opposite Scroll To Bottom?
It looks like it works by using End

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello. I don't know how, but it already works by End... Strange... That's why I didn't apply this command.

Copy link
Member

Choose a reason for hiding this comment

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

Hello. I don't know how, but it already works by End... Strange... That's why I didn't apply this command.

That's what I noticed as well, End already worked correctly, perhaps we can omit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I think we can omit it. If somebody will report issue, we can fix it easily.

vince-fugnitto
vince-fugnitto reviewed Feb 19, 2020
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I have a small comment regarding the new abstract methods, please don't forget to squash :)
The functionality works well for me.

packages/terminal/src/browser/base/terminal-widget.ts
@@ -38,6 +38,16 @@ export abstract class TerminalWidget extends BaseWidget {

abstract onDidOpen: Event<void>;

abstract scrollLineUp(): void;
Copy link
Member

Choose a reason for hiding this comment

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

A breaking change note is required (changelog) for these new abstract methods.
Anyone currently implementing TerminalWidget will be broken by the following changes.

Copy link
Member

@akosyakov akosyakov Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nobody should implement TerminalWidget, we should rather get rid of boilerplate interface later: https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#classes-over-interfaces

In other words it looks fine if it is not optional.

RomanNikitenko
RomanNikitenko approved these changes Feb 19, 2020
View reviewed changes
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 - it works well for me!
nice improvement, @AndrienkoAleksandr !

@akosyakov akosyakov removed their request for review February 19, 2020 15:00
@azatsarynnyy azatsarynnyy mentioned this pull request Feb 20, 2020
Closed
16 tasks
@akosyakov
Copy link
Member

@AndrienkoAleksandr it looks good to me, do you plan to change something else?

@AndrienkoAleksandr
Copy link
Contributor Author

it looks good to me, do you plan to change something else?

I am going to merge it today, if you don't mind.

@akosyakov
Copy link
Member

@AndrienkoAleksandr as usual remove commits which fix issues introduced by a PR and feel free to merge

@AndrienkoAleksandr
Add terminal scroll commands.
46ad80f 
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr AndrienkoAleksandr merged commit 8935269 into master Feb 26, 2020
@AndrienkoAleksandr AndrienkoAleksandr deleted the addTerminalScrollCommands branch February 26, 2020 14:06
@AndrienkoAleksandr AndrienkoAleksandr mentioned this pull request Mar 10, 2020
Merged
1 task
@jankeromnes jankeromnes mentioned this pull request Mar 10, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@akosyakov akosyakov akosyakov left review comments

@RomanNikitenko RomanNikitenko RomanNikitenko approved these changes

Assignees
No one assigned
Labels
accessibility issues related to accessbilitiy keybindings issues related to keybindings terminal issues related to the terminal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndrienkoAleksandr @akosyakov @RomanNikitenko @vince-fugnitto