Merge lp:~tveronezi/juju-gui/hotkeys into lp:juju-gui/experimental

[Thiago Veronezi]

application hotkeys

The user can hit alt+e to open the environment view and alt+s to focus the
charm search field.

https://codereview.appspot.com/6849078/

[Benjamin Saller (bcsaller)]

It doesn't look like you used lbox for this, but I'm including a reply here.

http://yuilibrary.com/yui/docs/event/key.html has a key filtering spec

such that we can say things like

// require modifier keys with +(modifier)
Y.one('doc').on('key', composeMail, 'n+ctrl');

This seems to meet our needs here and will eliminate much of the code included in this diff. This way we can fire directly to handlers w/o multiple events per keydown to do indirection like you have now.

Looking forward to having keyboard shortcuts :)

review: Needs Fixing

[Thiago Veronezi (tveronezi)]

>>It doesn't look like you used lbox for this, but I'm including a reply
here.
Hmmm... strange. I think I did. Well... I will try again later.

>>Y.one('doc').on('key', composeMail, 'n+ctrl')
Ah! That's good. I used something I did for another project. I will switch
to the yui stuff. Thanks!

[]s,
Thiago.

On Thu, Oct 25, 2012 at 11:42 AM, Benjamin Saller <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> It doesn't look like you used lbox for this, but I'm including a reply
> here.
>
> http://yuilibrary.com/yui/docs/event/key.html has a key filtering spec
>
> such that we can say things like
>
> // require modifier keys with +(modifier)
> Y.one('doc').on('key', composeMail, 'n+ctrl');
>
> This seems to meet our needs here and will eliminate much of the code
> included in this diff. This way we can fire directly to handlers w/o
> multiple events per keydown to do indirection like you have now.
>
> Looking forward to having keyboard shortcuts :)
>
> --
> https://code.launchpad.net/~tveronezi/juju-gui/hotkeys/+merge/131382
> You are the owner of lp:~tveronezi/juju-gui/hotkeys.
>

[lp:~tveronezi/juju-gui/hotkeys]

210. By Thiago Veronezi on 2012-11-08

trunk merge

211. By Thiago Veronezi on 2012-11-19

trunk merge

212. By Thiago Veronezi on 2012-11-19

personal code review

[Thiago Veronezi (tveronezi)]

Please take a look.

[Thiago Veronezi (tveronezi)]

Reviewers: mp+131382_code.launchpad.net,

Message:
Please take a look.

Description:
application hotkeys

The user can hit alt+e to open the environment view and alt+s to focus
the
charm search field.

https://code.launchpad.net/~tveronezi/juju-gui/hotkeys/+merge/131382

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6849078/

Affected files:
   A [revision details]
   M app/app.js
   M app/index.html
   M test/index.html
   A test/test_app_hotkeys.js

[Madison Scott-Clary (makyo)]

Thanks for the branch, Thiago. I have a few minors, but one regression
that I'd like to see taken care of before I give a +1.

https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode128
app/app.js:128: // F1...F12 or esc
Minor: This was unclear to me at first, and I had to stare at the next
line for a while to understand. What you're testing for is any key that
is NOT a function key or esc without a modifier, correct? I think that
this might be worth clarifying.

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode145
app/app.js:145: this.show_environment();
I'm not sure this is the right way to go about this, as it causes a
regression. If I view a service, then hit alt+E, the environment view
is shown, but not navigated to, leaving the url, in my case,
http://localhost:8888/service/mysql/. This breaks back-button behavior.

* view one service, mysql
* hit alt+E
* view another service, wordpress
* hit back: get taken to viewing mysql, because that was the last thing
in the URL stack.

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
Perhaps Y.Lang.isFunction(next), here?

https://codereview.appspot.com/6849078/

[Thiago Veronezi (tveronezi)]

Thanks for your review, Matt!

https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode128
app/app.js:128: // F1...F12 or esc
On 2012/11/19 18:36:51, matthew.scott wrote:
> Minor: This was unclear to me at first, and I had to stare at the next
line for
> a while to understand. What you're testing for is any key that is NOT
a
> function key or esc without a modifier, correct? I think that this
might be
> worth clarifying.

Done.

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode145
app/app.js:145: this.show_environment();
On 2012/11/19 18:36:51, matthew.scott wrote:
> I'm not sure this is the right way to go about this, as it causes a
regression.
> If I view a service, then hit alt+E, the environment view is shown,
but not
> navigated to, leaving the url, in my case,
http://localhost:8888/service/mysql/.
> This breaks back-button behavior.

> * view one service, mysql
> * hit alt+E
> * view another service, wordpress
> * hit back: get taken to viewing mysql, because that was the last
thing in the
> URL stack.

Good catch. Thanks.
I've solved this issue by firing a 'navidateTo' event. So now instead of
this.show_environment(), we call this.fire('navigateTo', { url: '/' });

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
On 2012/11/19 18:36:51, matthew.scott wrote:
> Perhaps Y.Lang.isFunction(next), here?

Hmmm. Not shure. I just want to test if we have the next object (for
unit tests purposes). If the developer passes one thing other than a
function, we should just let the exception be thrown. wdyt?

https://codereview.appspot.com/6849078/

[Madison Scott-Clary (makyo)]

Thanks for the reply! +1 with the navigateTo change

https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode145
app/app.js:145: this.show_environment();
On 2012/11/19 19:07:54, thiago wrote:
> On 2012/11/19 18:36:51, matthew.scott wrote:
> > I'm not sure this is the right way to go about this, as it causes a
> regression.
> > If I view a service, then hit alt+E, the environment view is shown,
but not
> > navigated to, leaving the url, in my case,
> http://localhost:8888/service/mysql/.
> > This breaks back-button behavior.
> >
> > * view one service, mysql
> > * hit alt+E
> > * view another service, wordpress
> > * hit back: get taken to viewing mysql, because that was the last
thing in the
> > URL stack.

> Good catch. Thanks.
> I've solved this issue by firing a 'navidateTo' event. So now instead
of
> this.show_environment(), we call this.fire('navigateTo', { url: '/'
});

Plugged this in, works well. Thanks!

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
On 2012/11/19 19:07:54, thiago wrote:
> On 2012/11/19 18:36:51, matthew.scott wrote:
> > Perhaps Y.Lang.isFunction(next), here?

> Hmmm. Not shure. I just want to test if we have the next object (for
unit tests
> purposes). If the developer passes one thing other than a function, we
should
> just let the exception be thrown. wdyt?

I don't feel that strongly either way :) Feel free to keep it as is,
unless someone else minds.

https://codereview.appspot.com/6849078/

[Benjamin Saller (bcsaller)]

With the navigate change this looks fine. I might remove the //XXX:
comment at the top of activate keys as even though we were not able to
use the YUI framework for this it is working code and working code
doesn't need to cry out for developer attention.

I'd also add a card indicating that we need design on a page bound to
'?'|'h' or similar that can list the hotkeys (and prehaps a link to help
in the base application template). These are useful but not
discoverable.

With that card in place, +1

https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
On 2012/11/19 19:07:54, thiago wrote:
> On 2012/11/19 18:36:51, matthew.scott wrote:
> > Perhaps Y.Lang.isFunction(next), here?

> Hmmm. Not shure. I just want to test if we have the next object (for
unit tests
> purposes). If the developer passes one thing other than a function, we
should
> just let the exception be thrown. wdyt?

This is the 'next' route in the App.Router chain, the persistent views
need this to continue processing, but they'd either all need an 'if
(next) next();' guard or not.

https://codereview.appspot.com/6849078/

[lp:~tveronezi/juju-gui/hotkeys]

213. By Thiago Veronezi on 2012-11-20

code review

[Thiago Veronezi (tveronezi)]

Thanks for your review Ben!

[]s,
Thiago.

https://codereview.appspot.com/6849078/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/6849078/diff/1/app/app.js#newcode557
app/app.js:557: if (next) {
On 2012/11/20 13:26:32, benjamin.saller wrote:
> On 2012/11/19 19:07:54, thiago wrote:
> > On 2012/11/19 18:36:51, matthew.scott wrote:
> > > Perhaps Y.Lang.isFunction(next), here?
> >
> > Hmmm. Not shure. I just want to test if we have the next object (for
unit
> tests
> > purposes). If the developer passes one thing other than a function,
we should
> > just let the exception be thrown. wdyt?

> This is the 'next' route in the App.Router chain, the persistent views
need this
> to continue processing, but they'd either all need an 'if (next)
next();' guard
> or not.

I fact, I dont need it now that I am calling the event instead of the
show_environment method directly. I will remove it.

https://codereview.appspot.com/6849078/

[Thiago Veronezi (tveronezi)]

*** Submitted:

application hotkeys

The user can hit alt+e to open the environment view and alt+s to focus
the
charm search field.

R=matthew.scott, benjamin.saller
CC=
https://codereview.appspot.com/6849078

https://codereview.appspot.com/6849078/

=== modified file 'app/app.js'
--- app/app.js	2012-11-16 08:23:47 +0000
+++ app/app.js	2012-11-20 14:53:22 +0000
@@ -106,6 +106,85 @@
     },
 
     /**
+     * This method activates the keyboard listeners.
+     */
+    activateHotkeys: function() {
+      Y.one(window).on('keydown', function(ev) {
+        var key = [],
+            keyStr = null,
+            data = { preventDefault: false };
+        if (ev.altKey) {
+          key.push('alt');
+        } else if (ev.ctrlKey) {
+          key.push('ctrl');
+        } else if (ev.shiftKey) {
+          key.push('shift');
+        }
+        if (key.length === 0 &&
+            // If we have no modifier, check if this is a function or the esc
+            // key. It it is not one of these keys, just do nothing.
+            !(ev.keyCode >= 112 && ev.keyCode <= 123 || ev.keyCode === 27)) {
+          return; //nothing to do
+        }
+        keyStr = keyCodeToString(ev.keyCode);
+        if (!keyStr) {
+          keyStr = ev.keyCode;
+        }
+        key.push(keyStr);
+        Y.fire('window-' + key.join('-') + '-pressed', data);
+        if (data.preventDefault) {
+          ev.preventDefault();
+        }
+      });
+
+      Y.detachAll('window-alt-E-pressed');
+      Y.on('window-alt-E-pressed', function(data) {
+        this.fire('navigateTo', { url: '/' });
+        data.preventDefault = true;
+      }, this);
+
+      Y.detachAll('window-alt-S-pressed');
+      Y.on('window-alt-S-pressed', function(data) {
+        var field = Y.one('#charm-search-field');
+        if (field) {
+          field.focus();
+        }
+        data.preventDefault = true;
+      }, this);
+
+      /**
+       * It transforms a numeric keyCode value to its string version. Example:
+       * 16 returns 'shift'.
+       * @param {number} keyCode The numeric value of a key.
+       * @return {string} The string version of the given keyCode.
+       */
+      function keyCodeToString(keyCode) {
+        if (keyCode === 16) {
+          return 'shift';
+        }
+        if (keyCode === 17) {
+          return 'control';
+        }
+        if (keyCode === 18) {
+          return 'alt';
+        }
+        if (keyCode === 27) {
+          return 'esc';
+        }
+        // Numbers or Letters
+        if (keyCode >= 48 && keyCode <= 57 || //Numbers
+            keyCode >= 65 && keyCode <= 90) { //Letters
+          return String.fromCharCode(keyCode);
+        }
+        //F1 -> F12
+        if (keyCode >= 112 && keyCode <= 123) {
+          return 'F' + (keyCode - 111);
+        }
+        return null;
+      }
+    },
+
+    /**
      * @method initializer
      */
     initializer: function() {
@@ -472,7 +551,6 @@
               render: true,
               callback: function(view) {view.postRender();}});
       }
-      next();
     },
 
     /**

=== modified file 'app/index.html'
--- app/index.html	2012-11-19 18:49:43 +0000
+++ app/index.html	2012-11-20 14:53:22 +0000
@@ -74,6 +74,9 @@
     <script>
       YUI(GlobalConfig).use(["juju-gui"], function(Y) {
         app = new Y.juju.App(juju_config);
+        // We need to activate the hotkeys when running the application
+        // in production. Unit tests should call it manually.
+        app.activateHotkeys();
       });
     </script>
   </body>

=== modified file 'test/index.html'
--- test/index.html	2012-11-19 19:52:43 +0000
+++ test/index.html	2012-11-20 14:53:22 +0000
@@ -33,6 +33,7 @@
   <script src="test_endpoints.js"></script>
   <script src="test_application_notifications.js"></script>
   <script src="test_charm_store.js"></script>
+  <script src="test_app_hotkeys.js"></script>
   <script src="test_notifier_widget.js"></script>
 
   <script>

=== added file 'test/test_app_hotkeys.js'
--- test/test_app_hotkeys.js	1970-01-01 00:00:00 +0000
+++ test/test_app_hotkeys.js	2012-11-20 14:53:22 +0000
@@ -0,0 +1,63 @@
+'use strict';
+
+describe('application hotkeys', function() {
+  var Y, app, container, env, conn, testUtils, windowNode;
+
+  before(function() {
+    Y = YUI(GlobalConfig).use(
+        ['juju-gui', 'juju-tests-utils',
+          'node-event-simulate'], function(Y) {
+          windowNode = Y.one(window);
+          app = new Y.juju.App({
+            env: env,
+            container: container,
+            viewContainer: container
+          });
+          app.activateHotkeys();
+        });
+  });
+
+  afterEach(function() {
+    container.remove(true);
+  });
+
+  beforeEach(function() {
+    container = Y.one('#main').appendChild(Y.Node.create('<div/>')).set('id',
+        'test-container').append(
+        Y.Node.create('<input />').set('id', 'charm-search-field'));
+    testUtils = Y.namespace('juju-tests.utils');
+    env = {
+      get: function() {
+      },
+      on: function() {
+      },
+      after: function() {
+      }
+    };
+  });
+
+  it('should listen for alt-S events', function() {
+    app.render();
+    windowNode.simulate('keydown', {
+      keyCode: 83, // "S" key
+      altKey: true
+    });
+    // Did charm-search-field get the focus?
+    assert.equal(Y.one('#charm-search-field'), Y.one(document.activeElement));
+  });
+
+  it('should listen for alt-E events', function() {
+    var altEtriggered = false;
+    app.on('navigateTo', function(param) {
+      if( param && param.url === '/' ) {
+        altEtriggered = true;
+      }
+    });
+    app.render();
+    windowNode.simulate('keydown', {
+      keyCode: 69, // "E" key
+      altKey: true
+    });
+    assert.isTrue(altEtriggered);
+  });
+});