Remove component templates #491

tcstewar · 2015-07-16T20:54:37Z

This gets rid of the Component/Template distinction that complicates the server-side code.

Now that Page and GUI have been reorganized, the entire reason for Templates is gone. They were originally there because the config file was shared across multiple Viz objects, and so the config file was processed before the actual Components themselves existed. So the config file actually configured these Component Templates, and the Components (when they were created when a page was actually generated) got their information from the Templates.

But now we don't look at the config file until after the Page is being created, so all that indirection is unnecessary, and so this PR gets rid of the Templates.

Some notes:

This slightly changes the .cfg file format, in that it is no longer necessary to refer to ValueTemplate in that file, and it's just Value instead. For backwards compatibility, ValueTemplate still works (otherwise all old .cfg files would fail).
There used to be two separate uids for the Component and the Template. The one for the component was just id(self) and was used by the websocket infrastructure to figure out which component the browser is trying to talk to. The one for the Template was the python string that could be used to refer to the object. This is the one used in the .cfg file and is consistent with all the other uid values in the rest of the system. In the new code, all uids in the Python code are always python strings for referring to things, and in the places where we need id(self) we just call id(self).

tcstewar · 2015-07-16T22:58:57Z

Another note: While fixing how this system interacts with the undo system, I accidentally discovered that boolean parameters on Components were being saved as 1 or 0 in the .cfg file. Fixing this solves #307

tcstewar · 2015-07-16T23:08:04Z

And this also fixes #431

Seanny123 · 2015-07-17T13:04:39Z

nengo_gui/components/netgraph.py

-class NetGraphTemplate(Template):
-    cls = NetGraph
-    config_params = dict()
+NetGraphTemplate = NetGraph


Although you removed the Template class, you still create a reference to the object named as the Template. Why is that?

Backwards compatibility?

Yes. I'll add a comment to make that clear (and perhaps find a way to do it in one place, rather than in each Component file).

Although you removed the Template class, you still create a reference to the object named as the Template. Why is that?

I've moved all that code to components/__init__.py and added a comment describing why it's there. :)

tcstewar · 2015-07-17T20:23:20Z

If this is going to be one of the final design-level iterations, it would be nice to have some comments/docstrings to specify the design-scope and purpose of the Component.

I've added comments to component.py and value.py. Let me know if those help!

Seanny123 · 2015-07-17T21:31:40Z

nengo_gui/static/netgraph.js

@@ -335,7 +335,7 @@ Nengo.NetGraph.prototype.on_message = function(event) {
        var uid = data.uid;
        for (var i = 0; i < Nengo.Component.components.length; i++) {
            if (Nengo.Component.components[i].uid === uid) {
-                Nengo.Component.components[i].remove(true);
+                Nengo.Component.components[i].remove(true, data.report_back);


report_back seems like a bit of an ambiguous variable name. Is the only use of this variable to check whether the item was being undone or deleted? Is it associated to a particular event and is it likely that it will always be associated to that event?

Hmm.. the intent is that it's a flag to indicate whether the NetGraph should report back to the server that the removal happened. We don't want it to do so in this case, as the server is the one who asked for it to be removed in the first place. Not sure what a better name would be.

How about server_removed?

Yuck... I usually prefer function parameters that indicate what the function is supposed to do, rather than function parameters that indicate the context in which someone else called the function. If report_back isn't clear enough, I'd be more tempted to do something like indicate_client_should_report_removal_back_to_server, but maybe not quite as silly as that. Can you say more about why you find report_back to be ambiguous? I'm having trouble seeing what's unclear about it.

Lol, Terry you have the most visceral reactions to bad naming suggestions.

Now that you've explained it to me, as well as your methodology for naming parameters, it makes sense to me. If it's not too much trouble adding a quick comment might be a good idea, especially considering how deep it is in the if-statements.

:) Gah, rereading my comment it does come off as a bit... visceral. Sorry about that.

And now that I'm rereading it, I wonder if part of it is that both "report" and "back" have many meanings, so it's unclear without a comment even how to parse this (is "report" a verb or a noun? Is "back" a noun or a verb particle?) So it's way more potentially confusing than I was initially believing.

So how about notify_server? That's a little less ambiguous to parse.... :)

That sounds delightful. (:

On Sun, Jul 19, 2015 at 10:42 PM, tcstewar notifications@github.com wrote:

In nengo_gui/static/netgraph.js
#491 (comment):

@@ -335,7 +335,7 @@ Nengo.NetGraph.prototype.on_message = function(event) {
var uid = data.uid;
for (var i = 0; i < Nengo.Component.components.length; i++) {
if (Nengo.Component.components[i].uid === uid) {

Nengo.Component.components[i].remove(true);

Nengo.Component.components[i].remove(true, data.report_back);

:) Gah, rereading my comment it does come off as a bit... visceral. Sorry
about that.

And now that I'm rereading it, I wonder if part of it is that both
"report" and "back" have many meanings, so it's unclear without a comment
even how to parse this (is "report" a verb or a noun? Is "back" a noun or a
verb particle?) So it's way more potentially confusing than I was initially
believing.

So how about notify_server? That's a little less ambiguous to parse.... :)

—
Reply to this email directly or view it on GitHub
https://github.com/nengo/nengo_gui/pull/491/files#r34966282.

Changed! :) Thank you for pushing on this....

Seanny123 · 2015-07-17T21:32:53Z

Other than my last two comments, this pull request LGTM.

tbekolay · 2015-07-20T18:58:40Z

nengo_gui/components/__init__.py

+# Old versions of the .cfg files used Templates which had slightly different
+# names than the Components currently usef.  This code is needed to
+# successfully parse those old .cfg files
+this_module = sys.modules[__name__]


I know this saves us a couple of lines, but this seems like a weird place to put in the mental effort to read/write this Python magic and realize what's going on. If I'm reading this all correctly, you could replace it with

SliderTemplate = Slider ValueTemplate = Value XYValueTemplate = XYValue # and so on

which is 9 lines versus these 4, but it definitely takes less time to read and understand the 9 line version. There's also no reason to bother doing this for new components, so it doesn't matter if it gets out of date. Ideally, we'll remove the 9 lines eventually anyway.

That is a really good point. :) I've updated it to use your much clearer approach.

tbekolay · 2015-07-20T19:05:02Z

nengo_gui/components/component.py

        self.replace_with = None

+    def initialize(self, page, config, uid):


initialize as a method name always make me cringe, since it seems like it should be done in __init__. There are times when you need to defer parts of what could be called initialization, but regardless I would not call the method initialize. If the docstring here is correct, then how about connect?

initialize as a method name always make me cringe, since it seems like it should be done in init.

:) fair enough.

I could see connect working, although that might be a bit ambiguous with the actual socket connections and nengo connections going on. Perhaps attach? or set_page?

I like attach, but I don't know about set_page since you're setting more than the page right? If you were just setting the page, I'd recommend making page a property, but it's more than that.

I just switched it over to attach. :)

tbekolay · 2015-07-20T19:14:31Z

Aside from my inline nitpicks, this looks great to me! Should be much clearer for those implementing new components, which hopefully will be several people ;)

tcstewar · 2015-07-21T20:48:34Z

I think I've responded to all the suggestions here.. anyone mind if I merge this? :)

Seanny123 · 2015-07-21T20:49:39Z

Go for it!

On Tue, Jul 21, 2015 at 4:48 PM, tcstewar notifications@github.com wrote:

I think I've responded to all the suggestions here.. anyone mind if I
merge this? :)

—
Reply to this email directly or view it on GitHub
#491 (comment).

Remove component templates

tcstewar added 3 commits July 16, 2015 14:46

First working version of removed templates

02a74b0

All graphs working without templates

68a87b7

Handled reloading

0c7f115

tcstewar added the needs review label Jul 16, 2015

tcstewar added 3 commits July 16, 2015 18:17

Fixed undo tracking

3e0d746

fix undo graph creation

ce295ed

Fixed saving boolean parameters to config

97bb1ab

tcstewar added 2 commits July 16, 2015 19:06

Fixed removing sliders that were created by undo

57ecff3

Fixed other plots causing Sliders to disappear

ff90339

Seanny123 reviewed Jul 17, 2015
View reviewed changes

tcstewar added 2 commits July 17, 2015 16:10

Cleaned up Template backwards compat; added docs

2997381

Added code comments for value.py

6d7e75c

Seanny123 reviewed Jul 17, 2015
View reviewed changes

tcstewar mentioned this pull request Jul 17, 2015

HTMLView component for custom visualizations #404

Merged

renamed report_back to notify_server

1d3923b

tbekolay reviewed Jul 20, 2015
View reviewed changes

Simplified backwards compatibility

5f3cdbd

tbekolay reviewed Jul 20, 2015
View reviewed changes

tcstewar added 4 commits July 20, 2015 15:27

renamed config_params to config_defaults

6636279

Fixed bug in saving Pointer graphs

babea3e

Renamed initialize to attach

7c5c95c

Simplified fix to pointer graph saving

5d96ffd

tcstewar added a commit that referenced this pull request Jul 21, 2015

Merge pull request #491 from nengo/remove-component-templates

13f46f0

Remove component templates

tcstewar merged commit 13f46f0 into master Jul 21, 2015

tcstewar deleted the remove-component-templates branch July 21, 2015 20:56

This was referenced Jul 22, 2015

Slider disappears when making value plot #431

Closed

Hidden state of labels isn't saved in cfg #307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove component templates #491

Remove component templates #491

tcstewar commented Jul 16, 2015

tcstewar commented Jul 16, 2015

tcstewar commented Jul 16, 2015

Seanny123 Jul 17, 2015

jgosmann Jul 17, 2015

tcstewar Jul 17, 2015

tcstewar Jul 17, 2015

tcstewar commented Jul 17, 2015

Seanny123 Jul 17, 2015

tcstewar Jul 17, 2015

Seanny123 Jul 17, 2015

tcstewar Jul 17, 2015

Seanny123 Jul 20, 2015

tcstewar Jul 20, 2015

Seanny123 Jul 20, 2015

tcstewar Jul 20, 2015

Seanny123 commented Jul 17, 2015

tbekolay Jul 20, 2015

tcstewar Jul 20, 2015

tbekolay Jul 20, 2015

tcstewar Jul 20, 2015

tbekolay Jul 20, 2015

tcstewar Jul 20, 2015

tbekolay commented Jul 20, 2015

tcstewar commented Jul 21, 2015

Seanny123 commented Jul 21, 2015

		self.replace_with = None

		def initialize(self, page, config, uid):

Remove component templates #491

Remove component templates #491

Conversation

tcstewar commented Jul 16, 2015

tcstewar commented Jul 16, 2015

tcstewar commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcstewar commented Jul 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seanny123 commented Jul 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbekolay commented Jul 20, 2015

tcstewar commented Jul 21, 2015

Seanny123 commented Jul 21, 2015