#460 ✓ staged
Bobby Richter

OpenMap plugin breaks when type isn't ROADMAP, SATELLITE, or TERRAIN

Reported by Bobby Richter | March 29th, 2011 @ 05:23 PM | in 0.6

There's no default value for options.type in the OpenMap plugin, and an exception is thrown as a result. I think this is the culprit:

        if( options.type == "ROADMAP" ) {
          // add OpenStreetMap layer
          projection = new OpenLayers.Projection( 'EPSG:900913' );
          displayProjection = new OpenLayers.Projection( 'EPSG:4326' );
          centerlonlat = centerlonlat.transform( displayProjection, projection );
          map = new OpenLayers.Map( { div: newdiv, projection: projection, "displayProjection": displayProjection } ); 
          var osm = new OpenLayers.Layer.OSM();
          map.addLayer( osm );
        }    
        else if( options.type == "SATELLITE" ) {
          // add NASA WorldWind / LANDSAT map
          map = new OpenLayers.Map( { div: newdiv, "maxResolution": 0.28125, tileSize: new OpenLayers.Size( 512, 512 ) } ); 
          var worldwind = new OpenLayers.Layer.WorldWind( "LANDSAT", "http://worldwind25.arc.nasa.gov/tile/tile.aspx", 2.25, 4, { T: "105" } ); 
          map.addLayer( worldwind );
          displayProjection = new OpenLayers.Projection( "EPSG:4326" );
          projection = new OpenLayers.Projection( "EPSG:4326" );
        }    
        else if( options.type == "TERRAIN" ) {
          // add terrain map ( USGS )
          displayProjection = new OpenLayers.Projection( "EPSG:4326" );
          projection = new OpenLayers.Projection( "EPSG:4326" );
          map = new OpenLayers.Map( {div: newdiv, projection: projection } ); 
          var relief = new OpenLayers.Layer.WMS( "USGS Terraserver", "http://terraserver-usa.org/ogcmap.ashx?", { layers: 'DRG' } ); 
          map.addLayer( relief );
        }    
        map.div.style.display = "none";

That last map.div.style.display happens even if map isn't instantiated.

This is problematic when automation is a concern. For example, Butter can't use this plugin because of this.

Comments and changes to this ticket

  • Rick

    Rick March 31st, 2011 @ 09:16 PM

    • State changed from “new” to “assigned”
    • Assigned user set to “Nick Doiron”
  • David Humphrey

    David Humphrey April 1st, 2011 @ 11:17 AM

    • Milestone changed from 0.5 to 0.6
    • Milestone order changed from “48” to “0”

    Nick, I'm pushing this out to the 0.6 release. If you get a patch in this week, feel free to switch it back.

  • Nick Doiron

    Nick Doiron April 1st, 2011 @ 01:49 PM

    I have updated https://github.com/mapmeld/popcorn-openmap-plugin to have OpenStreetMap / Roadmap selected unless satellite or terrain is specified, and to run that map.div line only if the map exists.

  • annasob

    annasob April 1st, 2011 @ 04:20 PM

    • State changed from “assigned” to “review-needs-work”
    • Milestone changed from 0.6 to 0.5

    Hey Nick,
    This looks great one nit pick however: please update the documentation at the top of the js file so that people know that ROADMAP is the default. PR+ with that fix.

    unit test and demo works in FF 3.6 4.0 and Chrome on Vista.

    Bobby can you ensure this solves your problem?

  • annasob

    annasob April 6th, 2011 @ 11:06 AM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user cleared.

    Im putting this into SR. I still want the default value added but lets see if SR turns out any other issues.

  • Nick Doiron

    Nick Doiron April 6th, 2011 @ 11:13 AM

    • Updated documentation that ROADMAP/OpenStreetMap is default

    • Also updated documentation which described map API as OpenStreetMap (it's OpenLayers + an open map source, including OpenStreetMap)

  • Steven W

    Steven W April 13th, 2011 @ 01:24 PM

    • State changed from “super-review-requested” to “under-review”
    • Assigned user set to “Steven W”
  • Steven W

    Steven W April 13th, 2011 @ 01:56 PM

    • State changed from “under-review” to “review-needs-work”

    The updates work great, Nick.
    Tested on Win7: FF 3.6, 4.0 and Chrome 10. Lint passes.

    One thing though, could you please add a unit test to check setting default values?
    After that, it looks golden.

  • Steven W

    Steven W April 13th, 2011 @ 01:56 PM

    • Assigned user changed from “Steven W” to “Nick Doiron”
  • Nick Doiron

    Nick Doiron April 13th, 2011 @ 02:11 PM

    Added unit tests that a new map without set layer:
    - defaults to OpenStreetMap - appears when expected - is removed when expected

    Increases count of expected positives by two

    https://github.com/mapmeld/popcorn-openmap-plugin

  • annasob

    annasob April 19th, 2011 @ 05:55 PM

    Nick,

    This fix is good however there is a tone of merge errors. Can you update the branch:

    git checkout master
    git pull git://github.com/annasob/popcorn-js.git 0.5
    git push origin master
    
  • Nick Doiron

    Nick Doiron April 19th, 2011 @ 07:33 PM

    I use a Git GUI on Windows and I'm not sure how to update. Complicated by my connection at a hostel in South America.

    Can anyone help?

  • annasob
  • annasob

    annasob April 19th, 2011 @ 07:54 PM

    Nick
    I believe what you want to do with git GUI is:

    1. add me to your remotes: remote->add (location: git://github.com/annasob/popcorn-js.git)
    2. checkout the branch you want to update: branch->checkout
    3. then do a fetch from me to get updates: remote->fetch from->[whatever name you added]
    4. merge in my stuff to your branch: merge->local merge->select tracking branch->select [whatever name you added]/0.5
    5. commit

  • annasob

    annasob April 21st, 2011 @ 05:25 PM

    • Milestone changed from 0.5 to 0.6
  • Nick Doiron

    Nick Doiron May 3rd, 2011 @ 05:27 PM

    Long overdue, updated my branch of the repo, updated OpenMaps plugin, result is here: https://github.com/mapmeld/popcorn-js

  • Rick

    Rick May 3rd, 2011 @ 06:34 PM

    • State changed from “review-needs-work” to “under-review”
  • Rick

    Rick May 3rd, 2011 @ 06:37 PM

    I was going to review this for you... but i can't figure out which branch your using, as there is no 460 or t460?

  • Nick Doiron

    Nick Doiron May 3rd, 2011 @ 09:53 PM

    I'm not sure what I did wrong. If you can explain the 460/t460 problem, I will try again.

    The only code which I've worked on is the plugin here - https://github.com/mapmeld/popcorn-openmap-plugin - and the update was made weeks ago. Maybe it'd be best if someone else took over this plugin so the code can be added.

  • David Humphrey

    David Humphrey May 3rd, 2011 @ 10:06 PM

    Hey Nick. It looks like you did a separate repo for your plugin. Rick was trying to find a branch name within a clone of our repo. so that's the confusion. Why don't you find me on irc tomorrow or some time this week and I'll guide you through getting this into proper shape. You might as well finish it after working so hard on it. Thanks!

  • Rick

    Rick May 3rd, 2011 @ 11:50 PM

    Nick, you didn't do anything wrong ;)

    Dave, Thanks for that clarification... Im going to be primarily AFK for the next few days, so I won't be much help.

  • Nick Doiron

    Nick Doiron May 4th, 2011 @ 06:00 PM

    I just set up a t460 branch; this seems to be the right way to link to it: https://github.com/mapmeld/popcorn-js/tree/t460

  • annasob

    annasob May 5th, 2011 @ 10:39 AM

    • State changed from “under-review” to “review-needs-work”

    Hey Nick, Thanks for making a branch
    This looks good but there is a couple of things that need changing:
    - openmap.html line 34 indent needed - openmap.js line 55 indent needed - I see a problem with line 66 in the same file. Since that id "actualmap" is used with Google Map. Lets change it to "opemmap" - for openmap.unit.js you do not need to use mapInterval = exec. Using exec is the right way to do it but exec() keeps its own state. Look at other plugins to see how it should look.

  • Nick Doiron

    Nick Doiron May 6th, 2011 @ 12:57 AM

    Made spacing and naming changes to the plugin, and believe I did what you're talking about to update my use of exec in the unit test. Let me know if that's working the right way.

  • annasob

    annasob May 16th, 2011 @ 04:25 PM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Nick Doiron” to “Scott Downe”

    Hey Nick,

    This looks good. The only problem is that you have if statements in the unit tests:

    popped.exec( 4, function() {
        if( popped.currentTime() > 3 && popped.currentTime() <= 5 ) {
          ok (document.getElementById('openmapdiv2'), "Second map is on the page" );
          plus();
          equals (document.getElementById('openmapdiv2').offsetParent.id, "map2", "Second map is inside the 'map2' div" );
          plus();
        }
      } );
    

    The exec function tracks the currentTime of the video so in the above code the function will be called when the video reaches 4 sec which means there is no need to verify the currenTime of the video . It should be:

    popped.exec( 4, function() {
     ok (document.getElementById('openmapdiv2'), "Second map is on the page" );
     plus();
     equals (document.getElementById('openmapdiv2').offsetParent.id, "map2", "Second map is inside the 'map2' div" );
     plus();
      } );
    

    Nick if you see this please fix. I am passing to SR for a second level review.

  • Rick

    Rick May 16th, 2011 @ 04:35 PM

    • Assigned user changed from “Scott Downe” to “Nick Doiron”

    This is leaking an unused var: openmapCallback;

    Instead of attaching 4 different .exec() calls, you could put all the tests into the same one (since they're all executing at the 4th second)

    +1 to annasob's review as well

  • Rick

    Rick May 16th, 2011 @ 04:36 PM

    • State changed from “super-review-requested” to “review-needs-work”
  • annasob

    annasob May 17th, 2011 @ 01:56 PM

    • State changed from “review-needs-work” to “assigned”
    • Assigned user changed from “Nick Doiron” to “annasob”

    Hey Nick you have done some awesome work here. I am gonna take your branch and finish up the last bits to get this staged. Feel free to take a look at the final commit to ensure i did not miss anything.

  • annasob
  • annasob

    annasob May 17th, 2011 @ 02:15 PM

    • State changed from “assigned” to “super-review-requested”
    • Assigned user changed from “annasob” to “Rick”

    Rick can u re-review this branch t460

  • Scott Downe

    Scott Downe May 17th, 2011 @ 04:39 PM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “annasob”

    Based on what I see in the ticket, and anna's patch, it looks like multiple execs are still being called on the 4th second. Why not just put them all in one exec?

    You only need two execs, one at 4 and one at 6 seconds.

    The global, unused leaky var is gone, though.

  • annasob

    annasob May 17th, 2011 @ 04:47 PM

    • State changed from “review-needs-work” to “super-review-requested”
  • annasob

    annasob May 17th, 2011 @ 04:49 PM

    • Assigned user changed from “annasob” to “Scott Downe”
  • Scott Downe

    Scott Downe May 17th, 2011 @ 04:54 PM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “Scott Downe” to “annasob”

    ok cool, we have the tests, and they are working in both ff and chrome.

  • annasob

    annasob May 17th, 2011 @ 05:00 PM

    • State changed from “review-looks-good” to “staged”
    • Assigned user changed from “annasob” to “Scott Downe”

    Staged in annasob/popcorn-js commit

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages