Allow keyboard input for keypad by FloEdelmann · Pull Request #1138 · OpenLightingProject/ola

FloEdelmann · 2016-10-21T15:06:15Z

Until now, one had to click the buttons in the web interface to control the keypad. I've enabled keyboard input for this.

peternewman

Can you have a look at the readme here:
https://github.com/OpenLightingProject/ola/blob/master/javascript/new-src/README.md

You'll need to make the changes here:
https://github.com/OpenLightingProject/ola/blob/master/javascript/new-src/src/controllers/keypad_universe.js

Then rebuild and commit app.js, app.min.js and app.min.js.map using the instructions in the readme.

peternewman · 2016-10-21T15:37:36Z

+          case 'a':
+            $scope.input(' @ ');
+            break;
+          case '-':


I'd be inclined to leave out -, so we can do 1 THRU 10 - 5 + 15 in future, without confusing things.

Ok, I did not know about that.

It was popular on Strand, and probably other consoles. The above would select channels 1, 2, 3, 4, 6, 7, 8, 9, 10 and 15.

Not quite intuitive 😉

I already thought about adding support for comma-separated lists of channels / channel ranges, but that would be another pull request.

What else would you expect it to do (aside from maths)?

The comma-separated list? I'd expect it to do something like this:
1 THRU 3, 9 @ FULL would set channels 1, 2, 3 and 9.

I was meaning the Strand style. Your comma syntax would need 1 THRU 4, 6 THRU 10, 15 to get the same effect as my example.

I admit I did not read closely enough, now I get how the syntax works. Nevertheless, sometimes commas might be better, e.g. 1 THRU 10, 50 THRU 60 @ FULL. But one does not forbid the other, so why not consider both?

Yep, in that case a comma would be perfectly acceptable I'd think.

You can also do things like 1 THRU 10 - 3 THRU 5. I can't really think of a +/, equivalent for that that makes sense. I'm not sure in Strand land, but it might make sense to keep , for multiple entries at once, or we could use ; for that. So 1 @ 5, 2 @ 7 as one simultaneous statement.

peternewman · 2016-10-21T15:39:14Z

Thanks for this!

FloEdelmann · 2016-10-21T16:04:57Z

Should the directive also go into the file you mentioned?

peternewman · 2016-10-21T16:36:01Z

Possibly a question for @daveol but from my understanding of directives and given what he did with filters, I'd say make a new folder and put it in there, similar to this:
https://github.com/OpenLightingProject/ola/tree/master/javascript/new-src/src/filters

I'm assuming that directive can be used to grab focus anywhere in the UI we might need to?

FloEdelmann · 2016-10-21T16:51:56Z

Exactly. I'll do that then.

* created new directives directory * removed '-' as 'THRU' shortcut * fix code style

FloEdelmann · 2016-10-21T17:56:52Z

Travis failed again because of the GitHub outage.

peternewman

A few comments

peternewman · 2016-10-21T17:48:51Z

    'src/constants.js',
+    'src/directives/autofocus.js',
    'src/factories/ola.js',
    'src/filters/start_form.js'


While we're here, can I be cheeky and get you to fix this, it's a typo and should be start_from.

Ok, I'll fix it.

peternewman · 2016-10-21T17:49:40Z

+        var key = $event.key;
+
+        if ($event.altKey || $event.ctrlKey || $event.metaKey
+            || ($event.which == 0 && key != 'Enter' && key != 'Backspace')) {


Can we put the or on the line above please (leave the ($event indented by one from the ( above).

peternewman · 2016-10-21T18:04:04Z

+        var key = $event.key;
+
+        if ($event.altKey || $event.ctrlKey || $event.metaKey
+            || ($event.which == 0 && key != 'Enter' && key != 'Backspace')) {


Is there a constant for this 0 or anything?

$event.which is 0 for non-printable keys (like the F1 - F12 keys I had in mind). Those won't interfere with the input's value anyway, so we just exclude them.

peternewman · 2016-10-21T18:08:24Z

+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Allows an element to obtail focus whenever a variable is set to true.


SPaG obtain

peternewman · 2016-10-21T18:14:25Z

Here is what the old web UI used to do on the keypad front. It would be good to add > for THRU at least:
https://github.com/OpenLightingProject/ola/blob/master/javascript/ola/common/keypad_parser.js#L186

FloEdelmann · 2016-10-21T18:18:49Z

Yeah, > is good but I think * does not really suit here. I'd rather expect all channels for that.

* Typo form -> from * Typo obtail -> obtain * indention * added '>' as 'THRU' shortcut

peternewman · 2016-10-21T18:20:25Z

You'll need to add the new file here to fix Travis: https://github.com/OpenLightingProject/ola/blob/master/javascript/Makefile.mk#L22

peternewman · 2016-10-21T18:31:47Z

    javascript/new-src/src/controllers/setting_universe.js \
    javascript/new-src/src/controllers/header.js \
    javascript/new-src/src/controllers/overview.js \
+    javascript/new-src/src/directives/autofocus.js \


You'll need to change start_from to form here too.

Can we fix the sorting please, by folder then file; so move factories and filters up (and above img).

Sorry, I pushed the moment you commented. Wait for another commit ;)

FloEdelmann · 2016-10-21T18:36:32Z

Travis does still fail 😢

peternewman · 2016-10-21T21:39:44Z

Yeah * did all channels before (min channel to max channel).

peternewman · 2016-10-21T21:40:10Z

Travis is now working again, there are a chunk of JS lint issues you'll need to fix please:
https://travis-ci.org/OpenLightingProject/ola/jobs/169594371#L417

FloEdelmann · 2016-10-22T13:38:18Z

jshint and jscs are don't agree on the case indentation. When I comply with one, the other fails.

peternewman · 2016-10-22T17:30:38Z

I'm inclined to say we should stick with jscs as you've currently done, as that's our own custom JS style, rather than a default one. So then it's just a case of stopping jshint complaining.

It looks like this got resolved in jshint here, which should be in jshint 2.2.0 upwards:
jshint/jshint@c64fb5c

Which we're already running, so needs a little more digging.

This is the first switch/case in our JS code, hence the issue.

peternewman · 2016-10-22T17:35:22Z

Actually I think it may be the {} block within the case @FloEdelmann . Have you tried removing that?

FloEdelmann · 2016-10-23T13:45:32Z

I have actually never used {} blocks for case statements but added them after jscs complained.

peternewman · 2016-10-23T17:58:12Z

I've taken the {} back out, as we need to fix our JCSC config instead.

peternewman · 2016-10-23T19:32:12Z

Travis should be fixed by OpenLightingProject/javascript-style#9 .

peternewman · 2016-10-24T14:46:54Z

As promised, now all green.

FloEdelmann · 2016-10-25T12:11:53Z

Nice, thank you :)

peternewman · 2016-10-26T21:49:14Z

@daveol if you want to retrospectively review this, let us know if there are any issues.

@FloEdelmann I realised I forgot to regenerate the JS after fixing lint issues and it looks like you did the same. Would you mind updating them and pushing a new PR please.

FloEdelmann · 2016-10-26T22:46:43Z

Unfortunately, I won't find the time until maybe Monday. If it's still relevant then, just let me know.

Regenerate JS after we forgot in #1138

Allow keyboard input for keypad

e4fcda2

Until now, one had to click the buttons in the web interface to control the keypad. I've enabled keyboard input for this.

peternewman self-assigned this Oct 21, 2016

peternewman added this to the 0.11.0 milestone Oct 21, 2016

peternewman requested changes Oct 21, 2016

View reviewed changes

FloEdelmann added 2 commits October 21, 2016 19:36

Update source files

dc111d2

* created new directives directory * removed '-' as 'THRU' shortcut * fix code style

fix Gruntfile

64d2fe6

peternewman requested changes Oct 21, 2016

View reviewed changes

Fixed small things

bedd2d1

* Typo form -> from * Typo obtail -> obtain * indention * added '>' as 'THRU' shortcut

FloEdelmann added 2 commits October 21, 2016 20:22

fix Makefile

f6f3918

fix form -> from in Makefile

1a887a7

peternewman reviewed Oct 21, 2016

View reviewed changes

Fix ordering in Makefile

cf62f73

FloEdelmann and others added 2 commits October 22, 2016 15:41

fix === and !==

6863161

Merge branch 'master' into keypad

1177df8

Merge branch 'master' into keypad

dcb8c36

Remove the curly brackets

ac616a1

peternewman approved these changes Oct 24, 2016

View reviewed changes

peternewman merged commit ef9ab39 into OpenLightingProject:master Oct 24, 2016

FloEdelmann deleted the keypad branch October 25, 2016 12:11

peternewman added a commit that referenced this pull request Nov 2, 2016

Merge pull request #1145 from FloEdelmann/js-regen

75c1eed

Regenerate JS after we forgot in #1138

peternewman mentioned this pull request Jul 12, 2026

Add 0.11 news #2060

Draft

Uh oh!

Conversation

FloEdelmann commented Oct 21, 2016

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

FloEdelmann commented Oct 21, 2016

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

FloEdelmann commented Oct 21, 2016

Uh oh!

FloEdelmann commented Oct 21, 2016

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

FloEdelmann commented Oct 21, 2016

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FloEdelmann commented Oct 21, 2016

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

peternewman commented Oct 21, 2016

Uh oh!

FloEdelmann commented Oct 22, 2016

Uh oh!

peternewman Oct 21, 2016 •

edited

Loading

FloEdelmann commented Oct 23, 2016 •

edited

Loading