[OpenLayers-Dev] ButtonBase code review

[olivier.terral]

Hi Cameron,

Sorry  for my lack of rigour.
I learn everyday :)


Cameron Shorter a écrit :
> 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.
Done as suggested but probably there are still other 
> ---
> 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.
Done as suggested. MouseListener is renamed Button and EditingListener 
is renamed EditingButton
> ---
> 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.
Done as suggested, Button class has children DragPan, ZoomIn .... and 
all EditingButton
>
> Button will need null parameters for id, imageOn, imageOff, etc.
Done as suggested,
> ---
> 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.
Done as suggested.  I've added a cursor property in Button class , 
moreover I've added a imgDir  property  because there  are a conflict 
between  OL image directory and  MB image directory  so now we can 
choose which directory we want(   in MB: Button.imgDir=url of skinDir
                                                           in OL: 
Button.imgDir=OpenLayers.Util.getImagesLocation()  (default value).
> ---
> 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.
Done as suggested.
> ---
> 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.
Done as suggested, simple but effective, I hope :)
> ---
> 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++){...}
Done as suggested
> ---
> 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).
Done as suggested
> ---
> 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.
Done as suggested. 95%of  _addButton code was moved to Button class: 
-Button.div properties are initialized in the constructor of the  Button 
class
                                                                         
                                       -in ButtonBar.js, _addButton 
function just add the Button.div to the Buttonbar div
> ---
> 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.
Done as suggested. A property "selected" is added to Button class, 
default value to false. This property is only  used in addTools function 
in Buttonbar class
> ---
> Buttonbar._addButton:
> Why register for mouseup event on the button? It is not used. You can 
> also remove buttonUp() function.
Done as suggested. buttonUp() function removed
> ---
> 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();
Done as suggested.
> ---
> 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.
Done as suggested. _swapButtonImg has been  moved  to Button class and 
It is only used in turnOn() and turnOff() functions
>  
>

Moreover I've added a buttonbar property in Button class, this property 
is only used in the Button.setTool() function and probbaly useful if 
there are several Buttonbar.