Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c248a97
Link to Open Fixture Library
FloEdelmann Oct 13, 2017
8610528
Merge branch 'master' into patch-1
peternewman Oct 14, 2017
065aabd
add links to each OFL mode
FloEdelmann Oct 15, 2017
44672a2
update Closure build tools and compile app.js
FloEdelmann Oct 17, 2017
9fb1d19
Merge branch 'master' into patch-1
peternewman Oct 27, 2017
c9c2bd7
Merge branch 'master' into patch-1
peternewman Oct 27, 2017
123d415
exclude rdm.js from BUILD
FloEdelmann Nov 26, 2017
2495fff
call closure compiler from Gruntfile
FloEdelmann Nov 26, 2017
70685f0
fix lint and add linter option
FloEdelmann Nov 26, 2017
5d67555
Merge branch 'master' into patch-1
FloEdelmann Nov 26, 2017
ab59937
Merge branch 'master' into patch-1
FloEdelmann Nov 29, 2017
c9a3e28
Merge branch 'master' into patch-1
FloEdelmann Dec 22, 2017
5d6108e
Merge branch 'master' into patch-1
FloEdelmann Dec 29, 2017
6691d41
Merge branch 'master' into patch-1
FloEdelmann Jan 3, 2018
06006ff
Remove double-whitespace
FloEdelmann Jan 3, 2018
436e8a0
Merge branch 'master' into patch-1
FloEdelmann Jan 25, 2018
f40c907
Merge branch 'master' into patch-1
peternewman Jan 27, 2018
61a3515
Add closure-compiler to Travis
peternewman Jan 27, 2018
6872b28
Add closure-compiler to .travis-ci.sh
peternewman Jan 27, 2018
9cb9b2f
fix a lot of Closure compiler warnings
FloEdelmann Jan 27, 2018
b6d625f
source out app.displayCommand to pid_utils.js
FloEdelmann Jan 27, 2018
60a8664
Merge branch 'master' into patch-1
peternewman Jan 27, 2018
051d284
fix requested JSDoc change; set OFL base URL only once
FloEdelmann Jan 27, 2018
8ca247e
Merge branch 'patch-1' of github.com:FloEdelmann/rdm-app into patch-1
FloEdelmann Jan 27, 2018
dbf8743
Ooops, remove debug console.log()
FloEdelmann Jan 27, 2018
b38bd14
construct the URL in Python
FloEdelmann Jan 27, 2018
a5f5897
Merge branch 'master' into patch-1
peternewman Jan 31, 2018
49840d9
Merge branch 'master' into patch-1
FloEdelmann Feb 6, 2018
a089d43
move "fieldset exists?" checks out
FloEdelmann Feb 6, 2018
af0e3de
Merge branch 'master' into patch-1
FloEdelmann Feb 28, 2018
5f76a1d
update JSDoc comment types
FloEdelmann Mar 5, 2018
387be32
Add myself to contributors
FloEdelmann Mar 6, 2018
42dc9f2
Merge branch 'master' into patch-1
peternewman Mar 9, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis-ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ elif [[ $TASK = 'karma' ]]; then
elif [[ $TASK = 'js-lint' ]]; then
grunt --verbose lint
elif [[ $TASK = 'closure-compiler' ]]; then
grunt --verbose closure-compiler
grunt --verbose closure-compiler
elif [[ $TASK = 'data-check' ]]; then
./tools/make_manufacturer_data.sh > data/manufacturer_data.py && git diff --exit-code data/manufacturer_data.py
elif [[ $TASK = 'spellintian' ]]; then
Expand Down
65 changes: 37 additions & 28 deletions js_src/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
* Copyright (C) 2011 Simon Newton
*/

goog.provide('app.setup');

goog.require('app.displayCommand');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😕

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@peternewman peternewman Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

@FloEdelmann FloEdelmann Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in #209

goog.require('goog.dom');
goog.require('goog.events');
goog.require('goog.ui.Component');
goog.require('goog.ui.TableSorter');
goog.require('app.pid');

goog.provide('app.setup');

// These are populated in the page
app.MANUFACTURER_ID = null;
Expand All @@ -37,7 +38,7 @@ app.SOFTWARE_VERSIONS = [];
* @return {number} Negative if a < b, 0 if a = b, and positive if a > b.
*/
app.hexSort = function(a, b) {
return parseInt(a) - parseInt(b);
return parseInt(a, 16) - parseInt(b, 16);
};


Expand All @@ -61,45 +62,52 @@ app.toHex = function(n, padding) {

/**
* Hide a node.
* @param {!Element} node The HTML element to hide.
*/
app.hideNode = function(node) {
node.style.display = 'none';
}
};

/**
* Show a block node
* @param {!Element} node The HTML element to show as a block.
*/
app.showBlock = function(node) {
node.style.display = 'block';
}
};

/**
* Show a inline node
* @param {!Element} node The HTML element to show inline.
*/
app.showInline = function(node) {
node.style.display = 'inline';
}
};

/**
* Show a table row
* @param {!Element} row The HTML element to show as a table row.
*/
app.showRow = function(row) {
row.style.display = 'table-row';
}
};


/**
* Create a TD node and set the innerHTML property
* @param {!string} text Inner HTML for the new table cell.
* @return {!Element} The created TD element.
*/
app.newTD = function(text) {
var td = goog.dom.createDom('td');
td.innerHTML = text;
return td;
}
};


/**
* Make the model table sortable
* @param {!string} table_id
*/
app.makeModelTable = function(table_id) {
var table = new goog.ui.TableSorter();
Expand All @@ -114,6 +122,8 @@ goog.exportSymbol('app.makeModelTable', app.makeModelTable);

/**
* Set manufacturer and model ID
* @param {!string} manufacturer_id The manufacturer RDM ID.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @param {!string} model_id The model RDM ID.
*/
app.setIds = function(manufacturer_id, model_id) {
app.MANUFACTURER_ID = manufacturer_id;
Expand All @@ -123,6 +133,7 @@ goog.exportSymbol('app.setIds', app.setIds);

/**
* Set the software versions
* @param {!string} version_info The software versions.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

*/
app.setSoftwareVersions = function(version_info) {
app.SOFTWARE_VERSIONS = version_info;
Expand All @@ -132,6 +143,7 @@ goog.exportSymbol('app.setSoftwareVersions', app.setSoftwareVersions);

/**
* Display info for the currently selected software version
* @param {?Element=} element
*/
app.changeSoftwareVersion = function(element) {
// default to displaying the first version
Expand Down Expand Up @@ -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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@FloEdelmann FloEdelmann Jan 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@FloEdelmann FloEdelmann Jan 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it's fixed now.

app.showBlock(personality_fieldset);
}
} else if (personality_fieldset) {
app.hideNode(personality_fieldset);
}

Expand All @@ -188,15 +202,17 @@ app.changeSoftwareVersion = function(element) {
if (type_str) {
sensor_type = type_str + ' (0x' + app.toHex(sensor['type'], 2) + ')';
} else {
sensor_type = app.toHex(sensor['type'], 2)
sensor_type = app.toHex(sensor['type'], 2);
}
goog.dom.appendChild(tr, app.newTD(sensor_type));
goog.dom.appendChild(tr, app.newTD(sensor['supports_recording']));
goog.dom.appendChild(tr, app.newTD(sensor['supports_min_max']));
goog.dom.appendChild(tbody, tr);
}
app.showBlock(sensor_fieldset);
} else {
if (sensor_fieldset) {
app.showBlock(sensor_fieldset);
}
} else if (sensor_fieldset) {
app.hideNode(sensor_fieldset);
}

Expand All @@ -215,14 +231,16 @@ app.changeSoftwareVersion = function(element) {
a.innerHTML = param_name + ' (0x' + app.toHex(param['id'], 4) + ')';
a.href = ('/pid/display?manufacturer=' + param['manufacturer_id'] +
'&pid=' + param['id']);
goog.dom.appendChild(li, a)
goog.dom.appendChild(li, a);
} else {
li.innerHTML = '0x' + app.toHex(param['id'], 4);
}
goog.dom.appendChild(supported_params_list, li);
}
app.showBlock(supported_params_fieldset);
} else {
if (supported_params_fieldset) {
app.showBlock(supported_params_fieldset);
}
} else if (supported_params_fieldset) {
app.hideNode(supported_params_fieldset);
}
};
Expand All @@ -231,6 +249,7 @@ goog.exportSymbol('app.changeSoftwareVersion', app.changeSoftwareVersion);

/**
* Display the latest version for the element.
* @param {!Element} element
*/
app.setLatestVersion = function(element) {
var index = 0;
Expand All @@ -244,12 +263,13 @@ app.setLatestVersion = function(element) {
}
element.selectedIndex = index;
app.changeSoftwareVersion(element);
}
};
goog.exportSymbol('app.setLatestVersion', app.setLatestVersion);


/**
* Make the pid table sortable
* @param {!string} table_id
*/
app.makePIDTable = function(table_id) {
var table = new goog.ui.TableSorter();
Expand All @@ -263,14 +283,3 @@ app.makePIDTable = function(table_id) {
table.setSortFunction(5, goog.ui.TableSorter.alphaSort);
};
goog.exportSymbol('app.makePIDTable', app.makePIDTable);


/**
* Display a pid command
*/
app.displayCommand = function(json, element_id) {
var msg_structure = new app.pid.MessageStructure();
msg_structure.decorate(goog.dom.$(element_id));
msg_structure.update(json['items']);
};
goog.exportSymbol('app.displayCommand', app.displayCommand);
Loading