Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Experimental] Data test branch. #152

Merged
merged 32 commits into from Mar 29, 2012

Conversation

Projects
None yet
2 participants
Collaborator

nrn commented Mar 11, 2012

This pull request is intended for discussion, and probably requires more work before it's worth pulling, if this is even the direction we want to go.

This branch is based around a micro library I just made dataDash, the goal being to make client side data more transparent. This gives us the ability to see everything we are storing by inspecting the dom. It also exposes the information in a standards based way that can be consumed and manipulated by other libraries.

So I revamped client.coffee to use this library, and seeing what we were storing where I undid a lot of duplication, leading to a few improvements, including generating the show json from the current state of the dom when it's clicked.

Thanks again to @m-n for working on this with me. I'm not really sure where this will lead, but I'm definitely interested in input.

Collaborator

nrn commented Mar 11, 2012

This also did a drive by of my master, there will be a separate, clean pull request for Express: Cleanup soon.

Owner

WardCunningham commented Mar 12, 2012

I wonder if dataDash could collect some statistics on how data attributes are used in practice?

One advantage of MVC frameworks is that the models give you a sense of what you have to work with when adding features. Perhaps dataDash could serve a similar role by accumulating stats that would be visible in Firebug or Selenium. I'd even be willing to provide an optional string argument as a comment if it would make a dataDash report more useful.

I'm thinking this would just be a hash of objects holding counts and other things that might be easy (or fast) to collect. For example:

{
  "id": {"set": 145, "get": 650, "on": ["div", "p"], "nb": "random key, usually 8 bytes of hex"},
  "site": { ...
}
Collaborator

nrn commented Mar 12, 2012

Absolutely. I wouldn't want to write that out to the dom every time something changed, but we could expose it in the console as dataDash.stats() for example, which the program could then write out to the dom whenever it felt like it calling something like $('body').dataDash({stats: dataDash.stats()});? Does it sound like that would fit the bill?

Owner

WardCunningham commented Mar 12, 2012

Yes. I'd be happy with console.log(dataDash.stats()) too.

Collaborator

nrn commented Mar 13, 2012

First draft of collecting stats. You can get to it from the global context with wiki.dataDash.stats().

Collaborator

nrn commented Mar 14, 2012

Wow, we need to fix that script tag problem :) I've got stats reporting with a new plugin type, "stats" temporarily.

Owner

WardCunningham commented Mar 14, 2012

I haven't been able to get this to run in my Sinatra environment. A bit of this is due to my confusion working with remote branches in git. I think I have that right now. I'm guessing that the client code has taken on a new dependency that I'm not yet meeting from the Sinatra side. The test suite says:

Finished in 25.74 seconds
51 examples, 16 failures

I'm guessing that this requires a simple edit in layout. It would be awesome if you'd make that edit and double check it running the Sinatra version of the integration tests. Thanks.

Collaborator

nrn commented Mar 14, 2012

I've got two failures right now:

  1. viewing journal should highlight a paragraph when hovering over journal entry
    Failure/Error: first_paragraph.should be_highlighted
    expected #<Capybara::Element tag="div"> to be highlighted

    ./spec/integration_spec.rb:282:in `block (2 levels) in <top (required)>'

  2. viewing journal should highlight all paragraphs with all the same JSON id
    Failure/Error: first_paragraph.should be_highlighted
    expected #<Capybara::Element tag="div"> to be highlighted

    ./spec/integration_spec.rb:294:in `block (2 levels) in <top (required)>'

Collaborator

nrn commented Mar 14, 2012

All the tests are green.

Owner

WardCunningham commented Mar 14, 2012

I'm still struggling with this. Tests run green here now but chart and d3 plugins fail with undefineds. Probably need some mechanism to include tests with plugins and run them automatically. I'm going to have to back off on this pull for now until I can think up a validation strategy.

Now might be a good time for you to write some programmer documentation for the library. I couldn't understand what you were doing with the journal hovering (6ea6a61) so I'm still shy digging in and fixing things myself.

Collaborator

nrn commented Mar 14, 2012

Whoops, yeah, this is going to take a bit of work to get into shape around the plugins and stuff. I've just been playing with the dataDash stuff and exploring. If you want to go this direction i'll get this in shape to pull by the weekend.

Owner

WardCunningham commented Mar 14, 2012

I can't say if your on to something with dataDash but I'm sure willing to give it a try. The stats() seems like a big reward and that comes largely from going through a standard interface that is under our control. It might be interesting to float your approach past the jQuery community for comments. Here's a thread that might be related.

The lack of plugin test coverage is a train wreck waiting to happen. Accept my apology there. I'd like to see plugin documentation, examples and test cases included in a plugin package. I think I'll ping @stephenjudkins and see if he wants to help me out there where he's been so valuable before.

Collaborator

nrn commented Mar 15, 2012

Awesome. I think i'm on to something here, but I can't quite put my finger on it, it probably needs some more real world use to flesh it out. I'll do what I can to get everything working together right. Any tests you guys write would help a ton.

Collaborator

nrn commented Mar 16, 2012

Well, all tests are green, and everything I can find to fix is working.

Collaborator

nrn commented Mar 18, 2012

Let me know if you see any blocking issues in pulling this. As I continue to play with it I believe we're ready for master.

Owner

WardCunningham commented Mar 19, 2012

Worked hard on plugin issues Saturday: a2f012f. Didn't get to writing plugin tests. Have spent today looking into chrome's profiling capabilities. Looking for a way to feel good about this.

Collaborator

nrn commented Mar 19, 2012

Understandable. I'm feeling pretty good about it at this point, but it is a fairly big change :) I won't be offended if you want to put this off and grab Stephen's changes now, as long as you give me some idea as to how to make you feel better about it.

Collaborator

nrn commented Mar 21, 2012

All tests are green again, and looking good under visual inspection

I use who below to bind the thumb event handler.

I've expanded wiki.getData() inside this plugin because I'm expecting to improve the "magnetic" attraction between data and visualization.

I think it is really unfair to other developers for me to be checking in modules with no documentation, tests or examples. Please accept my apology.

Owner

nrn replied Mar 22, 2012

Whoops, I shouldn't have been changing things without thoroughly checking, thanks for catching me.

Collaborator

nrn commented Mar 22, 2012

http://dev.nrn.io:3000/view/welcome-visitors/sensors.c2.com/list-of-sensors/sensors.c2.com/daylight-sensor/view/d3-line/sensors.c2.com/shoplight-sensor/view/d3-line/sensors.c2.com/distortion-sensor/view/d3-line

if my dev server is up this is an example of visualizations finding data magnetically to the left, so far i'm liking it, but that example highlights how bad the page names as ID problem is. I'm going to go after that next, possibly with temporary random ids, but i'm going to have to test that a bit.

Owner

WardCunningham commented Mar 22, 2012

Wow. That's awesome.

I've hacked the sensors.c2.com build script to make better image links. I was just getting broken windows because it had site-relative image links. (We've dogged a lot of asset management complexity by using data urls but I haven't used them in this case.)

Sweet.

I had no idea jQuery had a predicate like :lt which seems ready-made to express this logic.

As a fine point, it should probably be an :le (less-than or equal) comparison since data on the visualization page should not be ignored. I didn't see :le when looking through jQuery docs. Bummer.

Collaborator

nrn commented Mar 22, 2012

It's actually looking at all of the .items that come before the visualization, including on the same page, however it doesn't see any that come after it on the same page. I think that introduces a lot of edge cases, with multiple data elements and charts on the same page, that I'm not sure how we want to handle, but if you can clearly define the behavior I can figure out how to implement it :)

Collaborator

nrn commented Mar 22, 2012

Nice fix on the images, makes the demo much nicer.

Collaborator

nrn commented Mar 22, 2012

I'm working through things trying to make the page id unimportant, once I'm sufficiently there I'll try to figure out what it should be instead of the slug.

Owner

WardCunningham commented Mar 22, 2012

I once thought that if you called up the same page from the same server twice then the first copy should just move to the expected position for the second. You've shown that doing a simpler thing (just having multiple copies) is valuable.

Collaborator

nrn commented Mar 22, 2012

Which I find terribly inconvenient, I was thinking I could just move to using site/slug as the id until earlier today.

Collaborator

nrn commented Mar 22, 2012

Well, this seems to be working well without a page id at all, any thoughts?

Warning: This is not good to pull at the moment, the sinatra server needs some work to pass data-site instead of id.

Help me understand why you pick out the zeroth element of dataDash('slug') and when you use the whole result.

Owner

WardCunningham commented Mar 23, 2012

I've enjoyed reading these commits but must admit that I lost track of what is going on around line 450. It might just be late. Do the dataDash stats look reasonable?

Collaborator

nrn commented Mar 23, 2012

So, that was a design choice to treat all dataDash get results as an array, which looks kind of ugly when dropped in to replace jQuery's .data method, which only ever returns one item. Think it should have been 0 on that one, will check in the morning.

Thanks :) Around 450 is some pretty dense stuff that gets triggered every time we get a popstate event, taking the information from the new url and redoing the pages in the dom to reflect it. The stats were making sense before this latest round of stuff, will look at them again with the new lack of id, etc, in the morning.

@nrn nrn commented on the diff Mar 23, 2012

client/client.coffee
@@ -466,23 +481,22 @@ $ ->
# URL or DOM
pagesInDom = ->
- $.makeArray $(".page").map (_, el) -> el.id
+ $('.page').dataDash('slug')
urlPages = ->
(i for i in $(location).attr('pathname').split('/') by 2)[1..]
@nrn

nrn Mar 23, 2012

Collaborator

I think pagesInDom above or locsInDom below make a good argument for dataDash operating on arrays of elements, it makes for a natural interface when most selectors return more then one item anyway. It does have the odd problem of requiring you to get single items out of the array when you want just one, however you have the ability to do powerful things to ranges of selectors, and not worry what happens if there are different elements in the dom then you expected, you can just always get the first or last item, even if you get more then you expected you get a sane result.

Owner

nrn commented on a15e53a Mar 23, 2012

We no longer need to retrieve the old page from local storage, because we have all the information we want in the dom, always up to date.

So just need to add the change to the journal, bring the information out of the dom, and save it to local storage.

Collaborator

nrn commented Mar 23, 2012

Oh, and the stats are looking fairly reasonable to me.

Owner

WardCunningham commented Mar 25, 2012

I had some time on saturday to try out this branch. It seemed to work fine with the node server but didn't work at all with the ruby server. Is there something more that ruby must do?

The symptom was that every page name turned to undefined. No javascript errors. The wiki.log trace showed undefined in the very first trace line. Have to revert to the main branch. Sorry I couldn't debug this further.

Collaborator

nrn commented Mar 25, 2012

Yeah, It's probably a small change, we need the ruby server to add data-slug="slug" to the page div elements instead of setting the id. But I don't really know my way around haml, or ruby.

Collaborator

nrn commented Mar 25, 2012

Giving this a shot

Collaborator

nrn commented Mar 25, 2012

So this seems to work, everything looks good and all the integration tests passed. However I broke a bunch of ruby unit tests that seem to expect specific html that is now different.

Collaborator

nrn commented Mar 25, 2012

All systems go, all tests green, looking good poking at it.

Owner

WardCunningham commented Mar 25, 2012

Works lots better. Found a couple of things.

  • The colorful icon in a FederatedWiki plugin don't work. Here is what I'm talking about: http://ward.fed.wiki.org/view/federation
  • The specs that go straight to the ruby server are looking for id. Should be simple update.
Collaborator

nrn commented Mar 25, 2012

Hmm, don't seem to have that problem.

Owner

WardCunningham commented Mar 25, 2012

Those federatedWiki citations have two links in one. The blue word goes through the usual search of appropriate servers starting with origin. The little icon however goes straight to that server skipping the search.

I notice that there are some .data calls in the federatedWiki plugin.

Collaborator

nrn commented Mar 25, 2012

Oh, wow, didn't even notice those did that.

Collaborator

nrn commented Mar 25, 2012

fixed

Owner

WardCunningham commented Mar 25, 2012

Its good to get caught up. Haven't seen anything break with this latest fix. (except the latest commits on master.)

Collaborator

nrn commented Mar 25, 2012

Got the latest stuff in :) It's still looking good to go to me. Let me know if there is any thing I can do to put this on the path towards merging into master.

nrn added some commits Mar 25, 2012

Allow .json files to be retrieved cross origin
Also fixes an old bug where clicking on a plugin type
in the factory navigated to the right.
Collaborator

nrn commented Mar 26, 2012

Fixes the biggest problem in #168

Owner

WardCunningham commented Mar 28, 2012

I had another go at this merge this morning. I see the same symptoms I described in this comment above: all pages look to be named "undefined".

I checked that the divs that Sinatra generated include data-slug so I know the fix you put into the Sinatra server is working. I've tried every way to be sure Chrome is reloading all the Javascript. It does.

I checked out the code with these commands:

git fetch nrn
git checkout nrn/data-test

Followed by restarting the server.

Firefox seems to work just fine in this configuration so there is something complex going on with Chrome. I'm going to have to back out and do some other work but would appreciate suggestions.

Collaborator

nrn commented Mar 28, 2012

Sorry Ward, took another look at this again after we talked, and I can't reproduce it on any of my setups :/ Let me know if you want to pair and take a shot at this.

Owner

WardCunningham commented Mar 28, 2012

I put two hours into single-stepping and can't figure out what's wrong here. Sorry.

Collaborator

nrn commented Mar 28, 2012

Well, I'm not very confident without the broken setup to test against, but I took a shot in the dark. Just some minor improvements trying to make stuff that might be related less fragile. If you get a chance let me know if that makes it any better, tests are still good.

Let me know if I over reached with this one, would be easy to revert.

I generalized the radar chart data gathering stuff to a function that will return the data before a provided element, or the end, in order of closes to farthest, as a set of jQuery wrapped elements.

Owner

WardCunningham commented Mar 29, 2012

I really appreciate your patience. It is with intense pleasure that I finally merge these commits. Not only have you kept up with my exploratory visualization code that I've been dropping in here without any explanation, you've gently improved the parts where I didn't have time to think it through carefully. DataDash.js and D3.js will make quite a pair.

WardCunningham added a commit that referenced this pull request Mar 29, 2012

Merge pull request #152 from nrn/data-test
[Experimental] Data test branch.

@WardCunningham WardCunningham merged commit 0b594fb into WardCunningham:master Mar 29, 2012

Collaborator

nrn commented Mar 29, 2012

Your understanding and guidance on this branch have been invaluable, thank you! Man am I glad to have this pull request done.

Owner

WardCunningham commented Mar 29, 2012

Thanks.

There remains a mystery.

I'll call it the "all pages go to empty" mystery. We've seen that it is associated with a particular server, say localhost:1111, but does not affect other sites in the same farm, such as batch.localhost:1111.

Just now I've seen the mysterious behavior return, without provocation, to the one site on fed.wiki.org that I was planning to demo. No changes. Stops working.

I tell you this because I found a way to clear the condition, at least for now.

I went into Chrome's Clear Browsing Data screen and selected:

  • Clear download history
  • Empty the cache

It worked. Behavior returns to normal. I chose these because they sounded like the sort of thing that might confuse Chrome if unwanted entries got stored somehow. Previously I'd learned that opening a new tab or even restarting the browser had no corrective effect.

Next time this happens I will clear one or the other store so as to narrow down the culprit. It might be good to think about what sort of trap code could be added early to the javascript. Recompiling javascript didn't help but reverting from these commits did help. I'm already hooked on the better history and data binding behavior so I'm not considering going back. But where there is mystery, there is often trouble.

Collaborator

nrn commented Mar 29, 2012

Doh! Sorry. Well, the mystery of what i'm going to work on now is solved...

Owner

WardCunningham commented Mar 29, 2012

I just had "all pages go empty" on another site. This time I cleared the problem by selecting only "Empty the cache". I have not correlated this to any particular event. I was simply calling up a familiar federated wiki site by typing its domain name in Chrome's search/location bar. It came up in the malfunctioning mode.

Collaborator

nrn commented Mar 29, 2012

Aargh. I spent the afternoon looking at this, with no real leads.

Owner

WardCunningham commented Mar 30, 2012

I'm open to suggestions for things to try next time it happens to me.

I'm guessing its going to happen once or twice a day.

I tried googling for "chrome cache clear fixes bug" and found this note. There might be other posts with more hints. One challenge is having enough of a hint as to what is going on such that one can formulate a good google query.

Collaborator

nrn commented Mar 30, 2012

Aha! I got one to do it for me. Well, I found our problem. For some reason the sinatra server seems to randomly serve up a very old version of client.js, not sure what the most direct way to a solution is though. But I got a fresh reload that came up *.fed.wiki.org/view//view//view, and the client.js didn't even have dataDash in it anywhere, even though the html was the modern version, and dataDash was downloaded.

Collaborator

nrn commented Mar 30, 2012

Sorry, I mean when trying to access sinatra server you end up with a very old version of client.js. It's more then likely not actually coming from the server.

Collaborator

nrn commented Mar 30, 2012

The broken one seems to get 200 ok size: from cache

Whereas the working one seems to get 304 not modified size: 159b

Owner

WardCunningham commented Mar 30, 2012

Well, I noticed it again when visiting a farm site on fed.wiki.org that I had not visited since before the pulling our most recent version to that server. The failure scenario must be:

  • visit site before updating client.js
  • update server with new client.js, restart the server
  • visit site after the update, observe failure due to old client.js

Now, where is the old client code beging cached?

  • I knew to shift-refresh the page. No help.
  • I watched the traffic in Chrome's inspector. Status 200 implied reload.
  • Stopping and starting Chrome made no difference.
  • But, clearing Chrome's cache fixes the problem

I'm not claiming to know the answer. I'm just reiterating observations from memory. (Real testers keep notes.)

Collaborator

nrn commented Mar 30, 2012

Yeah, something along those lines fit my experience as well. It seems important to note, that the last modified on the client.js my broken instance is running is "Wed, 14 Mar 2012 21:43:44 GMT"

Collaborator

nrn commented Mar 30, 2012

Well, my ruby guru says we should get off webrick for production, and recommends the heroku platform as a service.

Owner

WardCunningham commented Mar 30, 2012

If we got rid of the recursive web service calls then we could go back to thin. Is he volunteering to help?

Collaborator

nrn commented Mar 30, 2012

Well, he volunteered to answer questions :)

In this space I started to write about how this was probably just some http header stuff that we could probably fix with a little bit of research and inspection, but then I looked at the sinatra book on deployment http://sinatra-book.gittr.com/#deployment, it doesn't even mention anything other than heroku.

Let me know if there is anything I can help with here, I'm really not sure what to do.

Owner

WardCunningham commented Mar 30, 2012

After merging this code yesterday we found an issue in drag-and-drop between pages that needs more work. I've moved the merged code to a data-test branch in this repository. I've also restored the unmerged code as master and added a little fix that handles an unwanted extra array wrapped around items that might have been moved. Here is the procedure I used to make this change:

It is possible that forks of this repository will have to make some adjustments. We're here to help.

@nrn and I have chatted this over by phone and will get his hard work integrated as soon as possible, probably after a week of travel and talks for me next week.

Owner

WardCunningham commented Mar 30, 2012

I found it necessary to tune up some of my local repositories after changes here. Here is what I did to get my local repository back in sync:

git branch -m master troubled-master
git checkout origin/master -b master

This sets the old master aside and creates a new local master that is in sync with the GitHub repo. Sorry for the inconvenience.

@harlantwood harlantwood referenced this pull request Apr 12, 2012

Closed

Heroku deployability #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment