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

Adding commands+shortcuts for keyboard nav of tabs in dock panel. #1647

Merged
merged 5 commits into from Feb 9, 2017

Conversation

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 8, 2017

Fixes #1637

This works, but I have a few questions:

  • Have got the logic right for these methods on ApplicationShell?
  • What keyboard shortcuts to use (currently accel left+right)?
  • Right now the terminal swallows these keyboard shortcuts, how do I make the terminal ignore them.
@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Feb 8, 2017

@sccolbert for review

Another question: right now I don't do anything if a user tries to select the next tab and they are at the end of the active panel. Is there a reasonable way of iterating through the panels in the dock panel?

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Feb 8, 2017

I think I see the logic for going between bars in the dockPanel.tabBars() to switch between panels. Another question though.

I have the TabBar instance that contains the active widget/tab. I want to find the next or previous TabBar from the dockPanel.tabBars() iterator. What is the best way of doing that with arrays or sequences in phosphor?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 9, 2017

We usually call toArray if were are going to do any indexing on an iterator.
See phosphorjs/phosphor#185 for how we are going to handle capturing keydown events before input areas get them.

@ellisonbg ellisonbg force-pushed the dock-panel-shortcuts branch from 7ee4a1d to 69abd61 Feb 9, 2017
@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Feb 9, 2017

Woohoo!! I have fun with this one. It is all working. This makes it really easy to navigate the entire dock panel using Accel ArrowLeft and Accel ArrowRight. It will cycle through tabs in each panel and also through panels, including periodic boundary conditions. Should be ready to go!

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 9, 2017

Squeaked it in for 0.16! I'll test/review this now.

private _previousTabBar(): TabBar {
let current = this._currentTabBar();
if (current) {
let bars = toArray<TabBar>(this._dockPanel.tabBars());
Copy link
Contributor

@sccolbert sccolbert Feb 9, 2017

Choose a reason for hiding this comment

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

the <TabBar> type is not needed. toArray will automatically infer the generic type based on the argument.

private _nextTabBar(): TabBar {
let current = this._currentTabBar();
if (current) {
let bars = toArray<TabBar>(this._dockPanel.tabBars());
Copy link
Contributor

@sccolbert sccolbert Feb 9, 2017

Choose a reason for hiding this comment

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

ditto

{
command: ApplicationCommandIDs.activatePreviousTab,
selector: 'body',
keys: ['Accel ArrowLeft']
Copy link
Contributor

@sccolbert sccolbert Feb 9, 2017

Choose a reason for hiding this comment

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

do these shortcuts not conflict with anything else in the app?

Copy link
Contributor Author

@ellisonbg ellisonbg Feb 9, 2017

Choose a reason for hiding this comment

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

Not that I know of...

Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

Seconded.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Feb 9, 2017

Made a few comments. Looks reasonable.

if (current) {
let ci = current.currentIndex;
if (ci !== -1) {
if (ci < (current.titles.length-1)) {
Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

spaces around the binary operator

Copy link
Contributor

@sccolbert sccolbert Feb 9, 2017

Choose a reason for hiding this comment

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

While we're being nit-picky, you don't need the extra parens either. The - operator has a higher precedence than logical operators: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

Copy link
Contributor Author

@ellisonbg ellisonbg Feb 9, 2017

Choose a reason for hiding this comment

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

I will remove the extra parens. Can you clarify what you mean about the spaces around the binary operator?

Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

current.titles.length-1 -> current.titles.length - 1

Copy link
Contributor Author

@ellisonbg ellisonbg Feb 9, 2017

Choose a reason for hiding this comment

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

Nevermind, I understand...

if (ci < (current.titles.length-1)) {
current.currentIndex += 1;
current.currentTitle.owner.activate();
} else if (ci === (current.titles.length-1)) {
Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

idem

let prevBar = this._previousTabBar();
if (prevBar) {
let len = prevBar.titles.length;
prevBar.currentIndex = len-1;
Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

idem

let ci = bars.indexOf(current);
let prevBar: TabBar = null;
if (ci > 0) {
prevBar = bars[ci-1];
Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

idem (and two lines down)

let len = bars.length;
let ci = bars.indexOf(current);
let nextBar: TabBar = null;
if (ci < (len-1)) {
Copy link
Member

@blink1073 blink1073 Feb 9, 2017

Choose a reason for hiding this comment

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

more idems

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 9, 2017

This is really elegant, well done!

@ellisonbg
Copy link
Contributor Author

@ellisonbg ellisonbg commented Feb 9, 2017

K, should be good, thanks for the review!

@blink1073 blink1073 merged commit 750a5cc into jupyterlab:master Feb 9, 2017
2 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 9, 2017

🎉

@ellisonbg ellisonbg deleted the dock-panel-shortcuts branch Jun 14, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants