Bookmark Control

Removed calls to Rico out of the control.

You now apply Rico to the div in the following manner.

bookmarks = new OpenLayers.Control.Bookmark({ 'div':OpenLayers.Util.getElement('myBookmarks'), 'title': 'Bookmarks outside the map' });
map.addControl(bookmarks);
bookmarks.add("WASHINGTON DC","-77.338126,38.639689,-76.788809,39.189005");

OpenLayers.Rico.Corner.round( bookmarks.div , {corners: "tl bl tr br", bgColor: "transparent", color: "darkblue", blend: false});
OpenLayers.Rico.Corner.changeOpacity(bookmarks.contentDiv, 0.75);

this looks like a good patch, but I think it is more material for Addins directory

See http://trac.openlayers.org/wiki/Addins

Some comments:

• APIProperties: At first glance, it seems like you might consider making the 'limit', 'stack', 'activateOnDraw', and 'clearOnDeactivate' properties APIProperties as perhaps users will want to have access to them.

• APIMethods: same as above, but for 'maximizeControl', 'minimizeControl'

• destroy(): generally we put this method directly below the initialize() function in the code... unless there is a clone() in which case the order is initialize(), clone(), destroy()

• string for bbox: is there a reason why you have chosen to use strings as the bounds, both in the interface and in the storage? I think most OL users are used to using the OpenLayers.Bounds object. The way you have built the examples here (input text box) I can see maybe why you would have chosen this. However, I would imagine most people who would use this control would have some sort of point and click interface to it... in which case converting bbox's to strings only to be converted back again seems like double-work. At the very least, you should provide the user with an option to specify the bounds as an OpenLayers.Bounds object.

• addView(): this function seems superfluous to me, especially given the above note.

One more thing that occurred to me is that it might be helpful (though not necessary) to have some tests for the control.

...I think we'll have to think a bit on what's the best way to get that infrastructure set up. test.Anotherway in each addin directory? maybe.

In addition to Erik's comments:

• activate() should call the parent's method
• activate() should return true or false (see other controls)
• ND comments above showControls() mention LayerSwitcher

I have applied the suggestions from Erik, except for the tests. I'll have to figure out how to write one. This is my first Javascript class.

I used some existing controls as a base for some code fragments may still remain.

I removed the activate() method. Couldn't see where it was needed.

Also, should I replace the code here or let it stay in my sandbox?

yes, if you could please update this ticket with the new, final patch... that would be helpful for those of us reviewing.

Updated control code in this ticket.

Re: Bounds as string: The original intent changed but the string remained. I have recoded the control to allow a string or bounds object to be passed.

see: Addins/Bookmark

graduated. see source:/addins/bookmark