(This is a new thread which grew out of "Event painter highlight feature not working with original or detailed painter" http://groups.google.com/group/simile-widgets/browse_thread/thread/acbffeaa662722f8 )
Hi Mike, Thank you for the code spelunking. I agree with you, some (many?) of those changes to separate out css from the js were apparently not tested. I'm sure that contributor meant well, but it didn't work out so well: In addition to breaking highlighting, they also broke aspects of the event attributes color and color text. Also broke proper calculations of event layout when tape height was changed. (The tape heights were in css, but the layout calculations are done in js based on metrics from the theme.) See my notes in the Changes file for revs 1623, 1630. Unfortunately, I think it is too late to simply undo the rev. It touched a lot of files and things would have to be manually unwound. -- If the problems were noticed right after the check-in, then it could have been easily un-done at that time. But it is now November and the checkin was done on April 3rd, 7 months ago. (And many revs ago, too.) (This also shows the benefit of testing your Timeline projects against the latest versions. At a minimum, you should test with each release.) Also, it *is* a good thing to use css files as much as possible. The tricky part is that Timeline changes styles on the fly and makes use of their metrics in computations. So some styling should be in css, other styling should be expressed dynamically in event files and themes, and other styling should use either place. Eg I think it is a good thing that label and tape color can be affected in either the event file or the css file if the event has a specific classname set. Instead of trying to undo the rev: 1) As the project becomes more popular we clearly need more qa on the committers and their sw. Timeline and the other projects are subtle and complex sw libraries. This is why I've been writing test examples. 2) David is becoming more circumspect in granting commit rights, a good thing IMHO. But on the other hand, manually processing patch files is not a fun exercise and I don't have much time for it. So if and when there are more commit requests, we may need to move to commit reviews, either pre or post commit. Good news is that Google code has some features for this, http://code.google.com/p/support/wiki/CodeReviews 3) We need more example files to test features. If someone would like to make a simple test file for highlighting and filtering, that'd be great. Then we can make sure that the features work or get them to work again, as needed. I think it is good to specify the highlighting class rules in css, but, of course, the style has to be correctly applied when an event is highlighted. 4) Even more than test files, we need explicit testing, perhaps via http://selenium.openqa.org/ also Yahoo has info on this subject: http://yuiblog.com/blog/2008/10/20/video-zakas-yuitest/ If someone (could be YOU, the reader), would like to read up on js library testing, and get something up and running for Timeline, that'd be really terrific. Regarding fixing highlighting: 1) If anyone has the skillset, please develop a patch and I will look into it. 2) As mentioned above, please create a simple example Timeline html file and {xml or json} data source file. Use one of the current examples as a template. The file should demonstrate highlighting and filtering with all three types of painters, original (default), overview and detailed. 3) As I mentioned in a previous message, my project will include event hightlighting at some point, so you can also choose to wait until I get there. Thanks again for your time on Timeline. Regards, Larry On Nov 2, 6:00 pm, "Michael Osterman" <[EMAIL PROTECTED]> wrote: > Hi all, > > I've tracked this down to a check r1326 "separation of javascript and > css.":http://code.google.com/p/simile-widgets/source/detail?path=/timeline/... > > I checked out a clean copy of trunk, and it looks like this commit was the > one that killed the highlight function. Even uncommenting the four colors > doesn't fix everything, as only the overview band gets the highlighting, but > not the "zoomed in" version. > > I also grabbed the revision previous to this, and other behavior with regard > to highlighting, such as a setting a "color" property in the JSON/XML only > works on one band, but not all. > > Can those of you who are elbows-deep in the code review this commit and > evaluate whether the changes made should be rolled back? I think the intent > was good, but it looks like it wasn't fully tested for all the ways in which > it might break functionality. > > Best, > Mike > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "SIMILE Widgets" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/simile-widgets?hl=en -~----------~----~----~----~------~----~------~--~---
