[OpenLayers-Dev] ButtonBase code review

[olivier.terral]

Hi

I've made a lot of modifications like you've said cameron,

But I'm in front of a little problem, for EditingAttributes I've moved 
in a button code so It's not a control yet, because there is a conflict 
between the button div and the control div
so I'm not sure if  it's in  accordance  with OL philosophy.

Have a look please.

Next episode, the NoButton.js file, coming back soon

bye

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.
> ---
> 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.
>
>