[OpenLayers-Dev] ButtonBase code review

[Cameron Shorter]

Olivier,
If you are addressing my comments, could you please go through the list 
and reply to each comment with what you have done. Eg:
 > ---
 > Line 10 Suggest that you use "case" instead of "if .. else if .. else 
if".
Done as suggested.
 > ---
 > Line 20 Suggest that you use "case" instead of "if .. else if .. else 
if".
Not done. I prefer using "if .. else if .." because in this case there 
is only 3 cases and the use of "case" doesn't add any value
 > ---

Inserting comments ensures I can be more effective in my second review.

Comments follow:

----------------------------------


---
Regarding EditingAttributes and Buttons and Controls:
This is a discussion that probably should be discuss further than just 
this code review, but you have identified an interesting problem: How 
should we handle tools which trigger popups? (In OpenLayers, popups are 
treated as controls).
I suggest we have 2 classes. An AttributeList control and a Query 
button. The Query button creates and stores the AttributeList control 
when the button is initialised, and the AttributeList is rendered when 
the Query button is actioned (by mouseClick).
A web will need to be able to override the AttributeList defaults - 
something like:
queryButton.attributeList.setParams([width:200, height:100]);
---
I suggest renaming EditingListener to EditingButton.
---
EditingAttributes should really be called Query or QueryAttributes as 
(as far as I can tell, it doesn't allow Editing of attributes. I realize 
that we use this name incorrectly in the Vector branch. A 
EditingAttributes button should extend the QueryAttributes button and 
should be a seperately selectable button.
---
There are lots of hard coded values in EditAttributes. I realise that 
these come from the vector branch, and that I'm responsible for many of 
them. At some point, these should be cleaned up.
---

olivier.terral wrote:
> 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.
>>
>>
>
>


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