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

Initial version of previous active tab toggle. #4296

Merged
merged 14 commits into from May 19, 2018
Merged

Initial version of previous active tab toggle. #4296

merged 14 commits into from May 19, 2018

Conversation

@ghost
Copy link

@ghost ghost commented Apr 1, 2018

#4177

Here is an initial version of the functionality. Default keyboard shortcut is Ctrl Shift '

Notes

  1. If you never open a second tab, previousID will remain the empty string, and the command will throw an error. Not sure if I should handle this, as a few commands throw errors at application launch time, but none are critical.

  2. Suppose we have three tabs open, and the command toggles between tab 1 and tab 3. Now, say you close tab 1. The command will essentially not do anything until we click on another tab, since the widget is no longer in the tab bar.

Question

  1. In all of the menu creation functions menu.menu.commands is used with the exception of createTabsMenu. Here, app.commands is used. Why is this?

  2. The commandID declaration is in createTabsMenu in stead of the CommandIDs namespace. Should I change the command to tabmenu level in stead of application level (like activate-next-tab) and move the declaration to CommandIDs?

  3. Should I add the command to the palette?

  4. It would be useful to check that the tab we are trying to toggle is current docked (i.e. We are not trying to switch to some temporary widget that was unable to load some unsupported file format). Can I use isVisible for this?

const commandID = 'application:activate-previous-active-tab';
if (!commands.hasCommand(commandID)) {
commands.addCommand(commandID, {
label: 'Activate Previous Active Tab',
Copy link
Member

@ian-r-rose ian-r-rose Apr 2, 2018

I think this label is kind of confusing. Perhaps something like "Activate Previously Used Tab"?

if (!commands.hasCommand(commandID)) {
commands.addCommand(commandID, {
label: 'Activate Previous Active Tab',
execute: () => app.commands.execute(`tabmenu:activate-${previousId}`)
Copy link
Member

@ian-r-rose ian-r-rose Apr 2, 2018

You can check if previousId exists here. Something like

execute: () => previousId && app.commands.execute(`tabmenu:activate-${previousId}`)

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 2, 2018

This is looking great @siryog90! To address your specific questions:

Notes

  1. I think it is best to check for the case of an invalid id. I have commented where you can do that.
  2. I think what you describe sounds like reasonable behavior. We should probably do a check there as well for whether a widget has been closed. Another possibility would be to keep a stack of all the activated tabs, so that you could go further back in the history of tab activation if the previously used one is no longer available.

Questions

  1. No particular reason. They should be references to the same command registry.
  2. Hmm, I don't have a strong intuition here. I think it might be a bit more consistent to move it to CommandIds under tabmenu, but opinions may differ.
  3. Sure, feel free to add it to the command palette.
  4. I think isVisible might be the wrong check here, since the tab can be open, but hidden behind another tab. Instead, I think that isDisposed is what you are looking for.

@ghost
Copy link
Author

@ghost ghost commented Apr 3, 2018

@ian-r-rose Ok, so the command works fine, but here are a couple things I am not particularly fond of.

Issue 1

Say I open tab 1 (notebook), and then tab 2 (notebook). Now I can toggle between them. Now suppose I double click on some file that JupyterLab is unable to open. Then currentChanged is fired twice. First time oldValue is set to the notebook I am toggling from, and newValue is set to a FileEditor, which would have contained the contents of the file I tried to open. Once JupyterLab "realizes" it can't display the file contents, currentChanged is fired again. This time oldValue is our FileEditor, for which isDisposed is now set to true, and newValue is the original notebook.

Because of this, checking for isDisposed in currentChanged doesn't help. Any thoughts on how I can approach this, or where in the code base I should dig for answers?

Issue 2

To your point, "We should probably do a check there as well for whether a widget has been closed," I can use the find, or toArray algorithms in Phosphor to check that a tab is docked, before trying to switch to it, but do you know of a O(1) method (hashmap) in the code base for this?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 3, 2018

Hmm, I am not sure the best way to approach issue 1 at the moment, it is a tricky one. Let me think on that.
As for issue 2: you can keep a reference to previousWidget, rather than previousId. Then you can check whether it has been closed, and you still have access to the id.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 3, 2018

One possibility for your first issue (though it is a bit indirect, and I know that the API for this is going to change soon-ish): you an check if the new widget is a document, then only update previous when the document successfully loads. It would look something like

let widget = args.newValue;
if (!widget) {
  return;
}
let context = docmanager.contextForWidget(widget); // If the widget is a document, this will succeed.
if (context) {
  context.ready.then(() => {
    previousWidget = widget;
  });
} else {
  previousWidget = widget;
}

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 14, 2018

Let's look at what other IDEs use for this keyboard shortcut as well.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 14, 2018

Also, we should use probably prefer to use Accel rather than Ctrl so Mac users can use Command.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

@siryog90 I think we should move forward with this. The issue with failed opening of a tab is an edge case that we would like to fix, but it can be done as a follow-up.

Can you change Ctrl to Accel to be more OS-independent? Also, it would be nice to know if there are any other applications that use ' as a keyboard shortcut for this (if we can't find any points of comparison, I have no problem with the choice).

if (!commands.hasCommand(CommandIDs.activatePreviouslyUsedTab)) {
commands.addCommand(CommandIDs.activatePreviouslyUsedTab, {
label: 'Activate Previously Used Tab',
execute: () => previousId && app.commands.execute(`tabmenu:activate-${previousId}`)
Copy link
Member

@ian-r-rose ian-r-rose May 17, 2018

It occurs to me that we can also add an isEnabled function here, so that the menu item is greyed out when previousId is not set. Something like

isEnabled: () => !!previousId

@ghost
Copy link
Author

@ghost ghost commented May 18, 2018

@ian-r-rose Give it a go when you get a chance, and let me know what you think.
Regarding the issue we are facing with auto closing "error" tabs, if you think it is a priority, I'll start putting together a solution. Otherwise, I'd prefer to wait until the API changes that you mentioned are pushed and then return to the problem. Thoughts?

Regarding other software keyboard shortcuts. First thought is to check browsers. Chrome has an extension that provides the functionality. It uses alt+tab to open a list of the most recent (ordered) tabs, from which the user makes a selection. This type of a list is definitely something we can set up, but again, I'd rather the new API settle in first, in stead of hacking something together.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

@siryog90 Just took the most recent version out for a spin, and it works great! I have a couple of small suggestions, and then I think we should merge.


// Command to toggle between the current
// tab and the previously used tab.
if (!commands.hasCommand(CommandIDs.activatePreviouslyUsedTab)) {
Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

This if statement check is not necessary, and I think it can be safely removed, since the command will only be added once on plugin activation.

if (!commands.hasCommand(CommandIDs.activatePreviouslyUsedTab)) {
commands.addCommand(CommandIDs.activatePreviouslyUsedTab, {
label: 'Activate Previously Used Tab',
execute: () => previousId && app.commands.execute(`tabmenu:activate-${previousId}`)
Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

We can also grey out the menu item when there is no previously used tab by adding an isEnabled property here. Something like

isEnabled: () => !!previousId

Copy link
Author

@ghost ghost May 18, 2018

Regarding isEnabled, it would be nice to disable the command if the user manually 'x'-s out the previously used tab. I am wondering if there is an event that fires in such a scenario. I would then check if the id of that widget matches previousId and null out the latter.

The following works.

    const populateTabs = () => {
      menu.removeGroup(tabGroup);
      tabGroup.length = 0;
      let widgetDisposal = function (widget: Widget) {
        if (widget && widget.id === previousId) {
          previousId = '';
        }       
      };
      each(app.shell.widgets('main'), widget => {
        widget.disposed.connect(widgetDisposal);
        tabGroup.push(createMenuItem(widget));
      });
      menu.addGroup(tabGroup, 1);
    };

Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

That's a nice idea. We could probably do it with a disposed signal, as you suggest (there is no problem with more than one connection to a given signal). I wonder if it would be easier to scan through the widgets in the main area to see if they are still there. Something like

isEnabled: () => previousId && find(app.shell.widgets('main), widget => widget.id === previousId )

Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

Alternatively, since populateTabs is called every time that the layout is modified (including when tabs are closed), the find function I described could also do the job of clearing out the previousId string there. So it could be

    const populateTabs = () => {
      menu.removeGroup(tabGroup);
      tabGroup.length = 0;
      each(app.shell.widgets('main'), widget => {
        tabGroup.push(createMenuItem(widget));
      });
      if (!find(app.shell.widgets('main'), widget => widget.id === previousId) {
        previousId = '';
      }
      menu.addGroup(tabGroup, 1);
    };

Copy link
Author

@ghost ghost May 18, 2018

I like the alternative approach. One wouldn't necessarily expect people to have a ton of tabs open at one time, but are we o.k. with the possible time hit find introduces?

Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

I think it should be okay. In fact, we are already iterating over the main area widgets, so that check can even go into the each function above.

Copy link
Author

@ghost ghost May 18, 2018

  const populateTabs = () => {
    menu.removeGroup(tabGroup);
    tabGroup.length = 0;
    let isPreviouslyUsedTabAttached = false;
    each(app.shell.widgets('main'), widget => {
      if (widget.id === previousId) {
        isPreviouslyUsedTabAttached = true;
      }
      tabGroup.push(createMenuItem(widget));
    });
    menu.addGroup(tabGroup, 1);
    previousId = isPreviouslyUsedTabAttached ? previousId : '';
  };

Copy link
Member

@ian-r-rose ian-r-rose May 18, 2018

Yep, that would work nicely, I think. And it only involves iterating over the widgets once.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 18, 2018

I don't think we need to worry about the error tabs for now.

@ghost
Copy link
Author

@ghost ghost commented May 19, 2018

I think it is ready for a spin.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks great @siryog90, thanks for the hard work!

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 19, 2018

Thanks!

@ian-r-rose ian-r-rose merged commit e377acd into jupyterlab:master May 19, 2018
2 checks passed
@ghost
Copy link
Author

@ghost ghost commented May 19, 2018

@ian-r-rose Thanks for the guidance!

@ghost ghost deleted the tab-to-last-modified branch May 19, 2018
@idoDavid
Copy link

@idoDavid idoDavid commented Jul 15, 2018

@ian-r-rose @siryog90
Trying to use the ctrl+shift as written in the docs above, but it's not working for me, i'm staying on the same tab.

I'm on mac OS, tried also CMD + Shift and still, not working. I'm on version 0.32.1, is it suppose to be there?

Thanks ;)

@ghost
Copy link
Author

@ghost ghost commented Jul 16, 2018

@idoDavid The feature is not released yet.

@jasongrout jasongrout added this to the 0.33 milestone Jul 19, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@idoDavid
Copy link

@idoDavid idoDavid commented Jul 31, 2018

@siryog90 I'm on 0.33.4 - Cmd shift and Ctrl shift is doing nothing. Any docs about how to enable and use this feature?

@ghost
Copy link
Author

@ghost ghost commented Jul 31, 2018

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jul 31, 2018

Thanks for tracking that down @siryog90, I think that removal was probably a mistake. I can submit a PR to reinstate it.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 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.

None yet

5 participants