Link to Open Fixture Library #128

FloEdelmann · 2017-10-13T07:16:32Z

closes #127

peternewman · 2017-10-13T19:21:34Z

The tests are failing as you're blocked behind #129 .

fxedel · 2017-10-14T07:01:35Z

Thanks for this quick review and approval! However, as described in OpenLightingProject/open-fixture-library#288, we also want to add ~~manufacturer and~~ personality links (the latter require some small changes on our side as well).

peternewman · 2017-10-14T07:22:02Z

To clarify here, I'm not sure manufacturer links to OFL particularly make much sense, it was more around getting some better workflows to avoid double collating some of the data.

The personality links are a good thing to have though.

Unfortunately while we're good at quick reviews @fxedel we still need to work on our deployment methods to improve and speed them up...

FloEdelmann · 2017-10-15T19:05:42Z

Is there a way I can test this locally? I installed all npm and grunt dependencies but couldn't find a deploy task or a html file that could be opened in Firefox so I tried to do everything "blindly".

peternewman · 2017-10-16T03:48:17Z

Sorry @FloEdelmann that bit doesn't seem to be very well documented.

That gets the JS code going, you then need to run the AppEngine code. See here for some documentation:
https://cloud.google.com/appengine/docs/standard/python/tools/using-local-server

Once you've run it up, you'll need to browse to http://localhost:8080/admin and click lots of the buttons to load manufacturers, PIDs, generate the PID indexes, and then load devices etc so you'll have some data to work with.

FloEdelmann · 2017-10-16T08:27:19Z

Okay, I got it up and running. Unfortunately, "Update Devices" only adds one device (the BEGLEC NV LED BAR). Manufacturers, PIDs, Controllers, Splitters, Software and Ethernet Nodes could be updated without problems.

So I still couldn't test the personality links since the LED BAR does not have RDM personalities 🤦‍
The fixture link works though.

Are you able to verify the personality links? Or did I make something wrong?

peternewman · 2017-10-16T17:39:57Z

We get most of our data via the model collector, and don't store the manually loaded stuff offline, hence the lack of test data. You can import this though:
https://github.com/OpenLightingProject/ola/files/1107996/robin_dl7s_profile.robe.txt

And I can probably dig out some other stuff (or you can capture more yourself). Just use the RDM Model Collector links and change the host/port part.

FloEdelmann · 2017-10-17T13:36:52Z

Okay, I now managed to work out how the Google Closure build tools have to be set up. I brought them up-to-date with npm's Closure packages. In order to compile successfully, I had to make some small changes to pid_display.js. There are still plenty of warnings that I didn't fix, but that can be part of another PR.

The code I wrote blindly seems to work fine, so I think this PR is now complete. Would you mind reviewing it @peternewman?

peternewman · 2017-10-31T16:27:23Z

Resulting HTML LGTM, code review to follow later today.

peternewman

Thanks for doing this.

A few minor comments.

Also given you've converted it to npm, are you interested in adding it to our Gruntfile given support seems to be built in. I think we could then just delete the BUILD file? https://github.com/google/closure-compiler-npm#using-the-grunt-task

BUILD

peternewman · 2017-11-01T02:15:23Z

js_src/app.js


 goog.provide('app.setup');

-var app = app || {}


It doesn't look like app is being defined anywhere now. Although I understand why you've gone down this route.

See my suggestion here about pulling some stuff out into another file to avoid the circular dependencies:
https://groups.google.com/d/msg/open-lighting/4bgqRxhg6XQ/d2pCA7RFAgAJ

Regardless where I put var app = {};, it complains about app already being defined in js_src/pid_display.js, where goog.provide('app.pid'); is called. And it does indeed work without explicitly including it, so I wouldn't put too much effort into this.

peternewman · 2017-11-01T02:43:28Z

Also it may make sense to add the linter argument to the compiler options:
https://github.com/google/closure-compiler/wiki/Lint-checks

Finally it would be great to run the whole lot during our Travis runs (which is pretty trivial if it's part of the gruntfile). Although I can make the Travis changes if you'd rather.

what a combination :)

FloEdelmann · 2017-12-22T15:01:35Z

@peternewman any updates on this?

peternewman · 2018-01-27T21:47:40Z

js_src/app.js

@@ -114,6 +122,8 @@ goog.exportSymbol('app.makeModelTable', app.makeModelTable);

 /**
 * Set manufacturer and model ID
+ * @param {!string} manufacturer_id The manufacturer RDM ID.


These want to be RDM manufacturer ID and RDM model ID please in the docs.

peternewman · 2018-01-27T21:54:18Z

js_src/app.js

@@ -17,13 +17,14 @@
 * Copyright (C) 2011 Simon Newton
 */

+goog.provide('app.setup');
+
+goog.require('app.displayCommand');


I was going to say you don't need this, but I'm guessing it optimises it out if it's not in there otherwise?

If it's not in there, I get a TypeError: app.displayCommand is not a function in the browser console on PID pages. However, I just noticed another error: TypeError: this.l.ya is not a function 😕

Hmm, then you have to look at the minification to make sense of it! Or perhaps try the non-advanced optimisation may be enough to tell, while giving it a real function name.

Ah, it's actually one of those warnings that were not produced by my changes: this.tt.setHtml is not a function

Ping @FloEdelmann . I've been doing some testing with deploy in #206 and I'm currently getting that "TypeError: this.l.ya is not a function" error and no tooltips e.g. on http://peter-ola-rdm-app.appspot.com/pid/display?manufacturer=0&pid=1 . It does work with what's on live though... http://rdm.openlighting.org/pid/display?manufacturer=0&pid=1

Prior to your changes it hadn't been touched since 2015, which is most likely what live is running.

It looks like this is why, and I suspect your bumping of closure-compiler and/or recompiling means we now need to update various bits such as: gmalartre/closure-library@8932858#diff-b655ee9f9748151f90992f5878791bf6L142

Hmmm, I didn't notice a consequence of that error when testing in this PR; seems like it does matter though (it even stops showing the upper_uid box).

Have you already tried just replacing this.tt.setHtml with this.tt.setSafeHtml here? https://github.com/OpenLightingProject/rdm-app/blob/master/js_src/pid_display.js#L122

In the second occurrence in that file (line 234), setText should be enough.

Both should be available (in contrast to the old setHtml), according to the Closure API.

fixed in #209

peternewman · 2018-01-27T22:02:05Z

js_src/app.js

@@ -166,8 +178,10 @@ app.changeSoftwareVersion = function(element) {
      goog.dom.appendChild(tr, app.newTD('<a href="' + oflLink + '">View in Open Fixture Library</a>'));
      goog.dom.appendChild(tbody, tr);
    }
-    app.showBlock(personality_fieldset);
-  } else {
+    if (personality_fieldset) {


I suspect these may be clearer if they're done as "if (personality_fieldset) { if (test) show or hide }", or given it's probably pointless populating the tbody if the fieldset doesn't exist, just test for that as an overall thing first. Equally should we be checking if tbody exists too, or is that not a warning?

tbody didn't raise a warning. It actually was because I added the explicit not-null {!Element} JSDoc type in the show / hide functions and goog.dom.$ returns a nullable Element. I agree on the clearer code style you suggested though.

Or shall we just do the null check in the show / hide functions themselves?

Doing it in the show/hide sounds sensible, although my gut feeling is that adding " if (personality_fieldset) {" as an initial statement outside the existing if may be clearer still, so we don't process tbody first.

Ok, it's fixed now.

peternewman · 2018-01-27T22:03:26Z

js_src/pid_display.js

 * @constructor
 * @extends goog.ui.Component
 */
-app.pid.MessageField = function(field_info, opt_domHelper) {


I think this should be able to switch back to app.pid.MessageField now the circle has been broken.

But it was just app.MessageField before my changes?

Ah sorry, I must have been looking at an intermediate diff before.

peternewman · 2018-01-27T22:07:36Z

Your base URL probably ought to be a constant too, given it's used in a few places (or I they between JS and template system)?

in Python, since this is the only way I can create new variables that can be passed to both the template engine and the client side JavaScript

this simplifies the whole process a lot

FloEdelmann · 2018-02-28T16:10:06Z

Is there now still anything preventing a merge?

peternewman

Just a few minor documentation comments

peternewman · 2018-03-03T15:59:52Z

js_src/app.js

@@ -114,15 +121,28 @@ goog.exportSymbol('app.makeModelTable', app.makeModelTable);

 /**
 * Set the software versions
+ * @param {!string} version_info The software versions.


Looking at https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System shouldn't this be !Array<string> or similar, actually looking at the contents, I think it's probably !Array<Object>?

peternewman · 2018-03-03T16:08:25Z

js_src/pid_display.js

 */
 app.MessageStructure.prototype.canDecorate = function(element) {
  return element.tagName == 'DIV';
 };


+/**
+ * @param {!Array.<app.MessageField>} fields


Can we be consistent please, either Array.< or Array< throughout.

Is there a place we used Array<something> instead of the standard JSDoc Array.<something> yet?

Hmm, perhaps just my suggestion in a comment here 😳 : #128 (comment)

peternewman · 2018-03-03T16:13:09Z

js_src/pid_display.js

 /**
 * A message field, this represents a field within a RDM message.
+ * @param {*} field_info


Can't this be tightened to an Object?

peternewman · 2018-03-03T16:13:17Z

js_src/pid_display.js

@@ -38,12 +40,14 @@ goog.inherits(app.MessageField, goog.ui.Component);

 /**
 * Return the underlying field info
+ * @return {*}


FloEdelmann · 2018-03-05T12:10:31Z

Now it should finally be ready 😀

peternewman

LGTM.

You might want to add yourself to contributors in package.json?

peternewman · 2018-03-06T04:34:22Z

Can you re-review this please @nomis52 , as it's changed quite a lot since you previously approved it?

peternewman · 2018-03-10T10:35:38Z

Thanks @FloEdelmann . Sorry it's taken so long to merge!

Link to Open Fixture Library

c248a97

closes #127

nomis52 approved these changes Oct 13, 2017

View reviewed changes

Merge branch 'master' into patch-1

8610528

fxedel mentioned this pull request Oct 14, 2017

Add RDM data to fixtures OpenLightingProject/open-fixture-library#288

Closed

add links to each OFL mode

065aabd

update Closure build tools and compile app.js

44672a2

peternewman added 2 commits October 27, 2017 09:47

Merge branch 'master' into patch-1

9fb1d19

Merge branch 'master' into patch-1

c9c2bd7

peternewman requested changes Nov 1, 2017

View reviewed changes

FloEdelmann added 6 commits November 26, 2017 23:25

exclude rdm.js from BUILD

123d415

call closure compiler from Gruntfile

2495fff

fix lint and add linter option

70685f0

what a combination :)

Merge branch 'master' into patch-1

5d67555

Merge branch 'master' into patch-1

ab59937

Merge branch 'master' into patch-1

c9a3e28

FloEdelmann added 4 commits December 29, 2017 19:14

Merge branch 'master' into patch-1

5d6108e

Merge branch 'master' into patch-1

6691d41

Remove double-whitespace

06006ff

Merge branch 'master' into patch-1

436e8a0

Merge branch 'master' into patch-1

60a8664

peternewman reviewed Jan 27, 2018

View reviewed changes

FloEdelmann and others added 8 commits January 27, 2018 23:57

fix requested JSDoc change; set OFL base URL only once

051d284

in Python, since this is the only way I can create new variables that can be passed to both the template engine and the client side JavaScript

Merge branch 'patch-1' of github.com:FloEdelmann/rdm-app into patch-1

8ca247e

Ooops, remove debug console.log()

dbf8743

construct the URL in Python

b38bd14

this simplifies the whole process a lot

Merge branch 'master' into patch-1

a5f5897

Merge branch 'master' into patch-1

49840d9

move "fieldset exists?" checks out

a089d43

Merge branch 'master' into patch-1

af0e3de

peternewman requested changes Mar 3, 2018

View reviewed changes

update JSDoc comment types

5f76a1d

peternewman approved these changes Mar 6, 2018

View reviewed changes

peternewman requested a review from nomis52 March 6, 2018 04:33

FloEdelmann and others added 2 commits March 6, 2018 07:59

Add myself to contributors

387be32

Merge branch 'master' into patch-1

42dc9f2

peternewman assigned nomis52 Mar 10, 2018

nomis52 approved these changes Mar 10, 2018

View reviewed changes

peternewman merged commit 3b65c41 into OpenLightingProject:master Mar 10, 2018

FloEdelmann deleted the patch-1 branch March 10, 2018 14:59

FloEdelmann mentioned this pull request Jun 25, 2018

Fix ClosureJS error #209

Merged

peternewman mentioned this pull request Sep 19, 2018

please (optionally) rebuild minified javascript OpenLightingProject/ola#1013

Open

Link to Open Fixture Library #128

Link to Open Fixture Library #128

Uh oh!

Conversation

FloEdelmann commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternewman commented Oct 13, 2017

Uh oh!

fxedel commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternewman commented Oct 14, 2017

Uh oh!

FloEdelmann commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternewman commented Oct 16, 2017

Uh oh!

FloEdelmann commented Oct 16, 2017

Uh oh!

peternewman commented Oct 16, 2017

Uh oh!

FloEdelmann commented Oct 17, 2017

Uh oh!

peternewman commented Oct 31, 2017

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman commented Nov 1, 2017

Uh oh!

FloEdelmann commented Dec 22, 2017

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 Jun 25, 2018 • 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!

FloEdelmann Jun 25, 2018 • 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!

FloEdelmann Jan 27, 2018 • 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!

FloEdelmann Jan 27, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FloEdelmann commented Oct 13, 2017 •

edited

Loading

fxedel commented Oct 14, 2017 •

edited

Loading

FloEdelmann commented Oct 15, 2017 •

edited

Loading

peternewman Jun 25, 2018 •

edited

Loading

FloEdelmann Jun 25, 2018 •

edited

Loading

FloEdelmann Jan 27, 2018 •

edited

Loading

FloEdelmann Jan 27, 2018 •

edited

Loading