terminal: add toggle terminal command

[jaimemartinagui]

What it does

Partial implementation of a new command and shortcut (keybinding) to toogle the integrated terminal in Theia. There are 4 cases for the command:

• Case 1: if no terminal exists, a new one is created.
• Case 2: if a terminal exists, but is hidden, expand the bottom panel and set the focus to it.
• Case 3: if a terminal exists and is visible, but without the focus, set the focus to it.
• Case 4: if a terminal exists, is visible and has the focus, collapse the bottom panel and set the focus back to the editor.

The part of managing the focus is still not implemented.

How to test

Try the shortcut ("ctrl+shift+.") in the different scenarios.

Review checklist

• [√] As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

There is no need to get all widgets and so on, you already should have access to the currentTerminal or lastUsedTerminal:

const currentTerminal = this.currentTerminal;
const lastUsedTerminal = this.lastUsedTerminal;

I believe this should be enough for all use-cases.

[jaimemartinagui]

Okay, I will try this option then and let you know. Thanks!

[jaimemartinagui]

I have modified this part and it is now using this._currentTerminal instead of retrieving all the widgets.

However, I am still having issues with the focus, to switch it between the terminal and the editor.

[vince-fugnitto]

@jaimemartinagui thank you for continuing to work on the pull-request 👍

I believe the following code should handle all use-cases:

protected toggleTerminal(): void {
    // Collect terminals from the bottom area.
    const terminals = this.shell.getWidgets('bottom').filter(w => w instanceof TerminalWidget);

    // If no terminal exists, open a new one.
    if (terminals.length === 0) {
        this.openTerminal();
        return;
    }

    // If the bottom panel is hidden, activate it and the first terminal found.
    if (this.shell.bottomPanel.isHidden) {
        this.shell.bottomPanel.setHidden(false);
        terminals[0].activate();
        return;
    }

    // If the bottom panel is visible, handle the visibility.
    if (this.shell.bottomPanel.isVisible) {
        const visibleTerminal = terminals.find(t => t.isVisible);
        if (!visibleTerminal) {
            this.shell.bottomPanel.activateWidget(terminals[0]);
        } else {
            this.shell.bottomPanel.setHidden(true);
        }
    }
}

[jaimemartinagui]

Hi @vince-fugnitto,

I have tested the code you provided and I found a slight difference in the behavior with the one I would expect (as it works in VS Code).

When the terminal is not hidden and has the focus, the terminal should be hidden, and it is in fact. However, when the terminal is not hidden and does not have the focus, I would expect this command to set the focus to the terminal. Instead of this, the code you provided hides the terminal. In order to modify this, I have changed the code to the following:

  protected toggleTerminal(): void {
      // Collect terminals from the bottom area.
      const terminals = this.shell.getWidgets('bottom').filter(w => w instanceof TerminalWidget);

      // If no terminal exists, open a new one.
      if (terminals.length === 0) {
          this.openTerminal();
          return;
      }

      // If the bottom panel is hidden, activate it and the first terminal found.
      if (this.shell.bottomPanel.isHidden) {
          this.shell.bottomPanel.setHidden(false);
          terminals[0].activate();
          return;
      }

      // If the bottom panel is visible, handle the visibility.
      if (this.shell.bottomPanel.isVisible) {
          if (this.shell.activeWidget && this.shell.activeWidget.id === terminals[0].id)
              this.shell.bottomPanel.setHidden(true);
          else
              this.shell.bottomPanel.activateWidget(terminals[0]);
      }

  }

Please let me know what do you think about this or if the other behavior is the intended one.

Thanks!

[vince-fugnitto]

However, when the terminal is not hidden and does not have the focus, I would expect this command to set the focus to the terminal.

@jaimemartinagui the behavior you identified is correct 👍 (I had not tested that use-case previously). However, I believe the code snippet you provided is problematic as it assumes that the first terminal is the visible one but not activated. Instead, you should check if there is a visible terminal in the bottom panel and compare it against the active widget.

Something like the following should work (it respects the visible terminal):

// If the bottom panel is visible, handle the visibility.
if (this.shell.bottomPanel.isVisible) {
    const visibleTerminal = terminals.find(t => t.isVisible);
    if (!visibleTerminal) {
        this.shell.bottomPanel.activateWidget(terminals[0]);
    } else if (this.shell.activeWidget !== visibleTerminal) {
        this.shell.bottomPanel.activateWidget(visibleTerminal);
    } else {
        this.shell.bottomPanel.setHidden(true);
    }
}

[jaimemartinagui]

Thank you @vince-fugnitto. That was something I realized after posting the comment. I have implementen this last change and pushed it to the "toogle-terminal" branch.

[vince-fugnitto]

@jaimemartinagui do you need any help to finish the pull-request, I believe the outstanding review is still #11193 (comment).

[jaimemartinagui]

@jaimemartinagui do you need any help to finish the pull-request, I believe the outstanding review is still #11193 (comment).

Hello @vince-fugnitto. I have already modified the keybinding as suggested by @colin-grant-work. I am not sure if I have to do anything else for the PR to proceed.

[msujew]

@jaimemartinagui The ECA check still fails. The website shows that you've commited with multiple email adresses, of which at least one isn't covered by the ECA:

Anonymous (jaimemartinagui@****.com)
Anonymous (64321866+jaimemartinagui@****.noreply)

The second one is likely an email address which was automatically created by GitHub. It might be best to rebase and squash your changes, so that only one address remains.

[jaimemartinagui]

@jaimemartinagui The ECA check still fails. The website shows that you've commited with multiple email adresses, of which at least one isn't covered by the ECA:

Anonymous (jaimemartinagui@****.com)
Anonymous (64321866+jaimemartinagui@****.noreply)

The second one is likely an email address which was automatically created by GitHub. It might be best to rebase and squash your changes, so that only one address remains.

So do you mean that I squash all the 14 commits of the branch in one by using interactive rebase?

[msujew]

So do you mean that I squash all the 14 commits of the branch in one by using interactive rebase?

Right, though there are other ways to accomplish this as well. You don't have to, but it might be the best way to resolve the ECA issue, especially since we don't allow noreply emails in our commits. We will squash your changes anyway when merging (we usually only perform one commit per PR), so you'll have more control over the final commit message/commit description if you just give us one commit to review.

[jaimemartinagui]

So do you mean that I squash all the 14 commits of the branch in one by using interactive rebase?

Right, though there are other ways to accomplish this as well. You don't have to, but it might be the best way to resolve the ECA issue, especially since we don't allow noreply emails in our commits. We will squash your changes anyway when merging (we usually only perform one commit per PR), so you'll have more control over the final commit message/commit description if you just give us one commit to review.

I have already squashed the commits into one single commit. Please let me know if it is now correct. Thanks!

[vince-fugnitto]

I have already squashed the commits into one single commit. Please let me know if it is now correct. Thanks!

@jaimemartinagui almost! :) It looks like there is a file which is unrelated to your changes packages/core/i18n/nls.zh-cn.json.

[jaimemartinagui]

I have already squashed the commits into one single commit. Please let me know if it is now correct. Thanks!

@jaimemartinagui almost! :) It looks like there is a file which is unrelated to your changes packages/core/i18n/nls.zh-cn.json.

Yes, I don't know why it appeared as modified that other file. I have solved the conflicts.

[jaimemartinagui]

I don't know why the build failed for macOS-10.15, note 16.

[vince-fugnitto]

I don't know why the build failed for macOS-10.15, note 16.

@jaimemartinagui no problem we can always re-trigger CI but there is a linting problem:

• https://github.com/eclipse-theia/theia/runs/7330056852?check_suite_focus=true#step:6:302

[jaimemartinagui]

There was ;; instead of ; in one line. Thank you!

[jaimemartinagui]

Hi @vince-fugnitto, could it be that the sign is missing, so the PR cannot be merced? or is there anything else blocking it?

[msujew]

@jaimemartinagui It seems like you didn't actually sign the ECA that's necessary for us to accept your changes. Signing the commit isn't necessary anymore (the documentation is a bit outdated).

[jaimemartinagui]

@jaimemartinagui It seems like you didn't actually sign the ECA that's necessary for us to accept your changes. Signing the commit isn't necessary anymore (the documentation is a bit outdated).

Thank you @msujew! I was not aware of it. I have already done it.

[colin-grant-work]

The cases described in the PR all work well for me, and the code looks correct. Thanks for the contribution!