Widgets #117

frankduncan · 2014-02-14T19:19:08Z

No description provided.

TheBizzle · 2014-02-27T19:25:50Z

Basically, I plan on not merging this until it has been merged into the two dependent repositories (NetLogo's headless branch, and Tortoise), but another thing that should be addressed before the merge is adding some CSS to the widgets to make them look consistent across browsers. This is something that Dan and I recently talked about, and he found it bothersome that the UIs difficult to modify, thanks to changes showing up differently in different browsers.

frankduncan · 2014-05-03T00:41:44Z

I added issue #133 so we can address your concerns later. I also updated it to use the latest version of tortoise (that doesn't pass tests due to a random push to headless unrelated to this), so you can merge this and put it out on linode if you like.

TheBizzle · 2014-05-14T21:44:05Z

@qiemem, please review the changes here to TortoiseJS.

TheBizzle · 2014-05-14T22:10:58Z

@frankduncan, I did some significant refactoring of your code and fixed a bug in widgets.coffee. Please review my changes.

Notable themes of my refactoring:

There were many unused imports in Widget.scala
The use of Scalaz was unnecessary and didn't really feel intuitive to me
There were several places where some or all of the indentation was wrong
Inline namespacing makes me upset (e.g. stuff similar to import org.nlogo.core; new core.Button)
Log level "error" was misused, according to my bible on the matter
I like to minimize code/value duplication (though, there's a certain point at which it's not worth it)
I like internal methods (like intOrFunc) to be declared at the top of their owner method, as I find it awkward to discover new methods in the middle of my method
I like common code between related lines to line up (e.g. keywords, equals signs, operators, etc.). I like this because I find it very easy to read code written in this way. Not doing this, I believe, is analogous to building a spreadsheet where each cell is differently sized.
I favor spending 10 lines storing into variables with clear names the parts of the string I'm building, and then combining them, rather than having 3 lines of dense code that builds string particles inline without being clear about what the particles are
I found Widget.compileFunc confusing in its implementation and its uses. The thing that confused me most about it was that it took two parameters, both of which were evaluated by name. This is a very unusual pattern for Scala code to take, and it is not immediately apparent why a compilation function would do this. The confusion interfered with my initial refactoring efforts, because I wanted to store the arguments into clear variable names, and this immediately changed the semantics of the code. The fact that by-name parameters have this curious effect of "the procedure cannot be replaced directly with the value" makes them not-so-desirable (usually). I eventually refactored the code so that this necessary evil would not make itself apparent whenever someone is implementing the toJS for a widget.

frankduncan · 2014-05-14T22:33:52Z

LGTM

TheBizzle · 2014-05-16T19:48:51Z

Beep, beep, @qiemem's a Jeep.

qiemem · 2014-05-22T18:49:19Z

app/assets/javascripts/TortoiseJS/agent/googlegraph.coffee

+    google.setOnLoadCallback(@drawPlot.bind(this))
+
+  plot: (y) ->
+    if @xpoint % 20 == 0


This looks like its only plotting every 20 points, which seems wrong.

frankduncan · 2014-07-02T18:04:50Z

Alright, should have everything accounted for at this point. @qiemem to review one final time. The dependency on the tortoise jar will relieve the NIL buttons and monitors

qiemem · 2014-07-03T19:13:19Z

app/models/local/WidgetJS.scala

+}
+
+trait CompilesCommands extends DoesCompilation {
+  override protected val defaultJS = "function() {}"


I don't think this is the correct default. The results of compileCommands are wrapped in a lambda already (see https://github.com/NetLogo/Galapagos/pull/117/files#diff-ca1195ec11f0f02b655a2a4238b02194R53), so you end up with function() { function() {} }, which isn't valid javascript. Rather, I think this should just be blank.

This is small stuff.... Just change it, yourself.

I wanted to make sure I was correct about the fix.

I did. It works. But I wanted to make sure it matched Frank's intention. Also, I wanted to put findings of review here, even if I'm going to fix them...

Looks correct to me!

Killer thanks.

Allos "save as html" to actually work.

qiemem · 2014-07-08T17:34:25Z

app/views/standaloneTortoise.scala.html

+      window.addEventListener('load', initPage);
+      if(useGoogleGraph) {
+        session.graph = GoogleGraph
+        Grapher.graph = GoogleGraph


Setting useGoogleGraph to true, I get Grapher is not defined here. @frankduncan, any ideas?

Okay, you can delete that line, but graphing definitely doesn't work by just doing that flag...

Label marks the number input. Also, this consequently supports people binding together an arbitrary number of widgets for custom sliders.

TheBizzle · 2014-07-16T20:22:33Z

@qiemem, it looks to me like it's mostly ready to merge. One thing that I noticed and found curious is that, while most sliders revert to their original values when loaded from "save as HTML", the speed slider does not. Is this intentional?

qiemem · 2014-07-16T21:01:09Z

That is not intentional. Good catch.

qiemem · 2014-07-17T15:46:45Z

Would it be better if all widget states were saved as displayed when a person saves?

TheBizzle · 2014-07-17T15:50:50Z

I was wondering about that, myself. It's not really clear. On the one hand, that's what NetLogo does (except for with the speed slider). On the other hand, I find that behavior really strange, and I think it runs counter to the purpose of the "Save as HTML" page. I don't feel totally strongly in one direction in another.

qiemem · 2014-07-17T16:12:41Z

In general, "save as html" is "capture what I'm looking at exactly as it is". I'm leaning towards saving state of everything, as that's more flexible, but I could easily be persuaded otherwise.

TheBizzle · 2014-07-17T16:38:47Z

I disagree. I see "save as HTML" as "take my existing NetLogo model and turn it into an HTML page"; the settings for how the sliders are "supposed to be" set is already in the .nlogo file. In my (anecdotal) experience, the sliders are adjusted on the page on the page before saving—not because of intent to change how the model will be run—but to test that the conversion worked correctly.

qiemem · 2014-07-17T16:52:45Z

I meant "Save as HTML" the browser feature rather than our feature, but I suppose it's probably not commonly used so people won't necessarily have expectations.

Saving the state of the widgets makes the feature more powerful and flexible, at the risk of potentially going against expectations. The problem is, without saving the states of widgets, there is no way for a user to be able to save a state without going back to the original model (without digging into the massive source file that results of course). That means, e.g., Concord as to come to us every time they want to change the default in their model. Though I suppose they'll hopefully just be able to generate models themselves at this point...

qiemem · 2014-07-17T22:26:38Z

I was persuaded on this by the idea that we shouldn't confuse people by having a kind of, sort of editor. It should conceptually be a model viewer, just as applets are.

qiemem · 2014-07-22T13:35:38Z

app/views/standaloneTortoise.scala.html


+      /*
      // If the HTML was saved in the non-default state, correct it.


@frankduncan What's up with all this commented out stuff? Can it be deleted?

Huh. Maybe I was keeping it as a reference? It looks like I did duplicate it above, but for some reason didn't actually delete the commented out stuff. So I'm guessing it can be deleted with prejudice

qiemem · 2014-07-22T15:16:21Z

Alright, everything looks good to me. Any objections before I merge this?

This is desktop NetLogo's behavior. See Virus for example

qiemem · 2014-07-22T21:06:27Z

Alright, I'm merging. Further problems should become issues.

Add basic widgets to create standalone

qiemem assigned qiemem and frankduncan and unassigned qiemem and frankduncan Mar 29, 2014

qiemem reviewed May 22, 2014
View reviewed changes

qiemem and others added 4 commits July 2, 2014 13:02

Fix speed slider so that models can go one tick at a time.

abb6795

Fix widget queries and netlogo-display attribute

4c0e837

Only create view if it doesn't already exist

184e8d5

Small changes for code review

5ff5745

qiemem reviewed Jul 3, 2014
View reviewed changes

qiemem added 2 commits July 3, 2014 15:54

Fix js defaults for commands and reporters

9a048bd

Prevent the creation of view layers if they already exist.

0b259aa

Allos "save as html" to actually work.

qiemem reviewed Jul 8, 2014
View reviewed changes

Make sliders out of both range and number inputs.

a241af3

Label marks the number input. Also, this consequently supports people binding together an arbitrary number of widgets for custom sliders.

qiemem reviewed Jul 22, 2014
View reviewed changes

qiemem added 2 commits July 22, 2014 10:14

Delete unused code

01fcb43

Make speed slider always default to middle

dfd35d9

Make monitors default to "N/A" when code throws runtime error

214788c

This is desktop NetLogo's behavior. See Virus for example

qiemem added a commit that referenced this pull request Jul 22, 2014

Merge pull request #117 from NetLogo/widgets

1f7a87e

Add basic widgets to create standalone

qiemem merged commit 1f7a87e into master Jul 22, 2014

TheBizzle mentioned this pull request Jul 24, 2014

Tortoise page throws errors into console #142

Closed

TheBizzle deleted the widgets branch October 6, 2014 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widgets #117

Widgets #117

frankduncan commented Feb 14, 2014

TheBizzle commented Feb 27, 2014

frankduncan commented May 3, 2014

TheBizzle commented May 14, 2014

TheBizzle commented May 14, 2014

frankduncan commented May 14, 2014

TheBizzle commented May 16, 2014

qiemem May 22, 2014

frankduncan commented Jul 2, 2014

qiemem Jul 3, 2014

TheBizzle Jul 3, 2014

qiemem Jul 3, 2014

TheBizzle Jul 3, 2014

qiemem Jul 3, 2014

frankduncan Jul 3, 2014

qiemem Jul 3, 2014

qiemem Jul 8, 2014

qiemem Jul 8, 2014

TheBizzle commented Jul 16, 2014

qiemem commented Jul 16, 2014

qiemem commented Jul 17, 2014

TheBizzle commented Jul 17, 2014

qiemem commented Jul 17, 2014

TheBizzle commented Jul 17, 2014

qiemem commented Jul 17, 2014

qiemem commented Jul 17, 2014

qiemem Jul 22, 2014

frankduncan Jul 22, 2014

qiemem commented Jul 22, 2014

qiemem commented Jul 22, 2014


		/*
		// If the HTML was saved in the non-default state, correct it.

Widgets #117

Widgets #117

Conversation

frankduncan commented Feb 14, 2014

TheBizzle commented Feb 27, 2014

frankduncan commented May 3, 2014

TheBizzle commented May 14, 2014

TheBizzle commented May 14, 2014

frankduncan commented May 14, 2014

TheBizzle commented May 16, 2014

Choose a reason for hiding this comment

frankduncan commented Jul 2, 2014

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

Choose a reason for hiding this comment

TheBizzle commented Jul 16, 2014

qiemem commented Jul 16, 2014

qiemem commented Jul 17, 2014

TheBizzle commented Jul 17, 2014

qiemem commented Jul 17, 2014

TheBizzle commented Jul 17, 2014

qiemem commented Jul 17, 2014

qiemem commented Jul 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiemem commented Jul 22, 2014

qiemem commented Jul 22, 2014