[OpenLayers-Dev] ButtonBase code review

[Cameron Shorter]

Olivier,
I have spent an hour or so reviewing your code in 
http://trac.openlayers.org/log/sandbox/oterral/ButtonBar/vector/lib/OpenLayers/. 
Overall, you have done a pretty good job. Ideally I'd like to spend 
more, so they are sure to be things I've missed.

Chris, I hope you will add comments too, especially about names of 
classes. Eg, What should we call the base Button/Tool class? I'm 
inclined to call it Button.js.

---
Minor: You have a few tab chars. Change them to spaces.
---
You have created tools in the directories:
lib/OpenLayers/MouseListener/ and 
lib/OpenLayers/MouseListener/EditingListener

I think the classes and directory structure should be renamed to 
emphasize  the fact that you are creating Button Tools. Rename 
MouseListener to Tool or Button, probably Button.
---
DragPan currently inherits from MouseListener, but MouseListener doesn't 
contain empty parameters for id, imageOn, imageOff, etc.
We should have a base Button class that all the Buttons/Tools inherit 
from. We have 2 options:
1. MouseListener has child Button which has children DragPan, ZoomIn, etc.
2. MouseListener is renamed to Button, Button has children DragPan, 
ZoomIn, ...
I prefer option 2. There will be Tools which don't have an icon and are 
always on (Eg, mouseTrack tool). I suggest we create a type "NoButton" 
type which can only be turned on/off from the program, not via operator 
interaction. They will default to on.

Button will need null parameters for id, imageOn, imageOff, etc.
---
Button should also contain a param for cursorIconType. Ie, when the tool 
is selected, the cursor type can change from an arrow, to a box, to 
something else. You should be able to find examples in the Mapbuilder 
code. I think it is in the MouseHander.js code.
---
Buttonbar.js (version 2281):
Why is X and Y initialised above the "inherit" braces? To be consistent 
they should be initialised with the rest of the variables.
---
Buttonbar.js:
Please add jsdoc comments to all functions. It makes it much easier to 
review. :)
Don't worry if your English has a French flavour to it. Us mono-language 
speakers are generally very forgiving. So long as your writing is 
understandable, then that is good. Someone else will clean up the 
comments later if there is a problem.
---
Buttonbar.addTools line 54:
Minor comment: When looping over an array with length>=0, I suggest 
using "for" instead of "while". This should be cleaner than the 
"if,while" statements you have starting at line 54.
for(var i=0;i<tools.length;i++){...}
---
Buttonbar.addTools line 78:
Minor: This block loops through all the current this.tools. I suggest 
that it only loop through the recently added tools. Ie, loop through 
tools. Why not move the code into the block above (ie from line 54).
---
Buttonbar._addButton:
The properties of btn should be moved into the base Button class, 
currently called MouseListener.
This means that the majority of _addButton will disappear.
---
Buttonbar._addButton:
I expected to see logic to ensure that one of the radio buttons is 
always selected.
By default, the first radioButton added should be selected.
---
Buttonbar._addButton:
Why register for mouseup event on the button? It is not used. You can 
also remove buttonUp() function.
---
Buttonbar.setTool:
You only seem to be catering for radioButton and button types. What 
about a twoState button?
I suggest code like:
switch(tool.type)
  case "radioButton":
    ..
    break;
  default:
    tool.doSelect();
---
Buttonbar._swapButtonImg:
The logic for rendering a button should be moved into the base button 
object, at the moment this is MouseListener.js.
An object should be responsible for drawing itself.


-- 
Cameron Shorter
Systems Architect, http://lisasoft.com.au
Tel: +61 (0)2 8570 5011
Mob: +61 (0)419 142 254