New issue
Advanced search Search tips

Issue 729789 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

nassh: close-on-exit doesn't exit

Project Member Reported by vapier@chromium.org, Jun 5 2017

Issue description

the window doesn't close automatically even when close-on-exit is set
 

Comment 1 Deleted

Comment 2 by vapier@chromium.org, Jun 29 2017

this is because the close-on-exit knob is checked here:
hterm_terminal.js:hterm.Terminal.prototype.runCommandClass:
...
  this.command = new commandClass(
      { argString: argString || '',
        io: this.io.push(),
        environment: environment,
        onExit: function(code) {   
          self.io.pop();
          self.uninstallKeyboard();
          if (self.prefs_.get('close-on-exit'))
              window.close();
        }
...

but the main users of runCommandClass implement their own exit logic on top of that:
nassh_command_instance.js:nassh.CommandInstance.prototype.exit:
...
  this.io.onVTKeystroke = (string) => {
...
      case 'e':
      case 'x':
      case '\x1b': // ESC
      case '\x17': // ctrl-w
        this.io.pop();
        if (this.argv_.onExit) {
          this.argv_.onExit(code);
        }
        break;
...

there's a bit of disconnect here between hterm's concept of "command" and nassh's concept of "command".  from hterm's pov, this is WAI: the nassh.CommandInstance hasn't exited yet, but once it does (by calling onExit), then the setting works correctly (the window closes).  from the user's pov, they see "ssh" as "the command", so when ssh exits, nassh doesn't check that, and waits for the user to hit "x".

i don't think we want to expose this level of detail to the user (i.e. making sep preference knobs).  the default behavior has been to set close-on-exit even though, from the user's pov, we haven't been doing that.  so most users will see a change in behavior when they upgrade.  i guess they'll get used to it, or go in and toggle the knob.

but then we see that, when close-on-exit is disabled, the user is one again exposed to the disconnect between these projects.  nassh will present the "exit" option which the user will choose, but then hterm won't actually close the window.

so the question is how to architect this so the higher level commands get access to this, while the user experience doesn't get funky.

we could have hterm pass in a callback when instantiates the commandClass.  something like:
  this.command = new commandClass(
      { argString: argString || '',
        io: this.io.push(),
        environment: environment,
        closeOnExit: () => self.prefs_.get('close-on-exit'),
        onExit: function(code) {   
          self.io.pop();
          self.uninstallKeyboard();
          if (self.prefs_.get('close-on-exit'))
              window.close();
        }
then nassh would check this.argv_.closeOnExit() and if it's true, call onExit immediately instead of setting up its fallback vtkeystroke handler.  it needs to be a callback rather than passing the value directly since the prefs object could update at runtime.

another option would be to add the callback to some hterm class (most likely hterm.Terminal since it has all the relevant runtime objects bound to it), but that doesn't feel like a good fit.  the closest class i can find is hterm.Options, but even that is geared towards terminal state rather than command state.

to deal with the nassh-exits-but-hterm-doesn't case, we can extend the onExit API to to take a 2nd argument like "close".  if it's true, we'll behave as if close-on-exit was set.  if it's undefined, then we'll check the close-on-exit knob.  this should preserve the API for other users of hterm.

we could split the callbacks into something like onExit() & close(), but i'm not sure this is an improvement over the existing API, and would cause backwards compat issues.
        close: () => window.close(),
        onExit: function(code) {   
          self.io.pop();
          self.uninstallKeyboard();
          if (self.prefs_.get('close-on-exit'))
            this.close();
        }
that way nassh could call onExit and then close when the user told it to exit.

Comment 3 Deleted

Comment 4 Deleted

Sign in to add a comment