HUE-141: Front-end for Shell app

Review Request #1719 — Created April 22, 2011 and updated

aditya
old-hue
HUE-141b
HUE-141
hue
andyao, marcus, nutron, shawn, Thomas
Here's the front-end portion of the Hue Shell. It's basically unchanged from what I had submitted earlier, but I'd like you all to take a look and see if any of these things are no longer how we do them. I'm not particularly thrilled about the code that creates the menu of available shell types, but what I want is a bunch of uniform-width buttons, and I couldn't figure out any other way to do that.

Features:
Supports multiple shells
Multiplexes one network connection for all instances of app (Hue.ShellPoller)
Bash-style command history if you push the up and down arrows.
If you wait for the POST /prefs/state request to occur after you start your shell, opening Hue and logging in as the same user in another browser (or browser tab) restores the shell, and you can type commands into either tab/browser. Output goes to both browsers.
Exponential backoff on failed requests (new!).

Known issues:
Blinking cursor doesn't always appear in Firefox (not sure if this is worth fixing, please chime in)
If you click the 'X' to kill a shell in one browser/tab while concurrently viewing it in another browser/tab, it is possible that the other doesn't immediately get that info. This has to do with how the backend is structured. This is a really rare case so I'm not worrying about it, but I can figure out how to solve it if you think it's important. In any case, if you try typing a command into a shell that you killed in another browser/tab, it then realizes that it was dead the whole time. This is a small regression.
Works in FF, Chrome, and IE8. Didn't test Safari.
nutron
  1. You have about half of this code commented nicely. You should run through it and add some docs for the rest.
  2. this.options - options is the argument, but this.options is the merge of the two. I realize that there isn't a default for shellId, but it's not expected that you won't use this.options here, and subsequent work on this app may include some change that would expect this convention be followed.
  3. ... = [];
    
    The constructor for native objects aren't used in JS and aren't recommended to be so. If you want to know why I can dig out my copy of JavaScript the Good Parts and point to it. The nutshell is that not all the constructors work as you'd expect. Example:
    
    !!new Boolean(false) == false
  4. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
    any reason that this can't just live in a css file?
    1. The fixed_width_font class can be moved to css, but the #ffffff can't be. When a process exits, the box is grayed out. As a result, if you then click on the icon in the window itself to reload the app, the box remains gray.
  5. I'd consider giving this container element a diff class name that actually indicates it's function (in addition to jframe_padded, which designates a style). .shell_container or something.
  6. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
    If this button is hidden why is it here? We typically add these buttons in the "web 1.0" version and hide them in Hue so that the web 1.0 version works, but this app doesn't have a web 1 version (I think?) and if it does it doesn't run this JS... I don't think you need this.
    1. Good point. Took this out.
  7. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    I don't love the duplication here; seems  like this should be one method with a flag for whether or not it's a restore.
  8. why not use your focusInput method here?
  9. any reason that this method (listenForShell) doesn't just take an instance of the shell and then reference the values it wants from it?
    1. I set it up this way thinking that the poller would only care about the next output offset and the shell id, and a callback. This way, the admin view could specify a different callback. However, it appears that we might not ship an admin view after all, so I'm putting this on hold.
  10. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    nix all this, use a UL/LI list w/ anchor tags display block w/ button class. attach a click event and call it a day.
    1. Done. I needed a container div as well, but that's what lives in the HUE-141b branch now.
  11. I think that instead of making the TD have this class I'd put a display:block anchor inside each one.
  12. and I'd make this click, which I think would make it possible to tab through these
  13. In theory  you can use the Keyboard instance on this jframe and register shortcuts. This will make the shortcuts show up in the shortcuts menu in Hue.
    1. That sounds like overkill, though, doesn't it? We don't want to be displaying the shortcuts if the app isn't open, or even worse, installed.
    2. No, it doesn't work that way. The shortcut view only displays the currently active shortcuts. So if you click on the filebrowser and then the shortcut list, you'll see shortcuts for window management and the filebrowser. Change context to something else (say, your shell app, or beeswax) and then click the shortcut list again and you get a different list. It's context aware.
  14. you can make this an object instead of a string concat if you like. If you make it an object, I don't think you have to worry about the URI encoding - request handles it. Double check me on that.
  15. you are adding this to ALL instances of Request, not just the shell app. Is this your intention?
    1. I really just need it for the requests from Hue.ShellPoller. I don't want to use an HTTP parameter here on principle. Is there a way to include the header only for the requests from Hue.ShellPoller?
    2. You have two choices: 1) always specify this option in the instances you create for the poller (simple enough) or 2) extend the class into a subclass with this option set.
  16. desktop/core/static/js/Source/Hue/Hue.ShellPoller.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
    all of these can just be instance properties on the prototype, no?
    1. Is there any special syntax for doing that in our JS stack, or is it okay to just do "this.prototype.foo = bar;" ?
    2. You just define them as properties in the class definition:
      
      var Foo = new Class({
        numAdditionalReqsSent: 0,
        additionalReqs: [],
        addToOoutputReqOpen: false,
        etc
      });
  17. desktop/core/static/js/Source/Hue/Hue.ShellPoller.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    generally, this patch needs a little more comments. This block is a good example of one that could use it.
  18. desktop/core/static/js/package.yml (Diff revision 1)
     
     
    Why is this in core/desktop?
    1. I need there to be a globally available poller. I'm perfectly happy with this not being in core/desktop, but all instances of the Shell app need to have an available poller they can share. Is there another way to do this?
    2. It should be in the poller app, not in core desktop. It's location shouldn't have any effect on how you use it.
  19. 
      
Thomas
  1. 
      
  2. apps/shell/src/shell/static/css/shell.css (Diff revision 1)
     
     
     
    "  " -> \t
    
  3. apps/shell/src/shell/static/css/shell.css (Diff revision 1)
     
     
     
     
     
     
     
    "    " -> \t
  4. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Actually, you don't even need a list.
    
    <div class="flowVertically">
        <a href class="round Button">foo</a>
        <a href class="round Button">bar</a>
        ...
    </div>
    
    .flowVertically > * {
        display: block;
    }
    
  5. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
    What's going on here?
    
    I guess I need to see a screenshot of this UI before I'll understand.
  6. I'll review the rest once I see what it is I'm reviewing ^_^
shawn
  1. I didn't read all of the code. Instead, I read the other reviews and noted the following few points:
    
    1. Style nit: Choose a convention for CSS class names. I see both dashed-names and underscored_names here. The front-end team has recently chosen a camelCase convention for class names, which helps prevent some types of collisions. We capitalize classes for standalone components, and don't capitalize classes for their subordinate parts, and for modifiers such as component state (which should be adjectives).
    
    2. Code style nit: We use spaces after "if", "else", "for", "while", etc. to distinguish them from function calls.
    
    See #3 below.
    
    
  2. apps/shell/src/shell/static/js/Source/Shell/Shell.js (Diff revision 1)
     
     
     
     
     
     
     
     
    3.
    a. Would offsetWidth be better than adding width and padding styles?
    b. Some browsers will choke on addition of these styles with "px" or other units. You have to strip the units before adding them, then append the unit again. And if the units don't all match, then you have to do unit conversion, which... isn't fun. Refer to comment #3a.
  3. 
      
Loading...