ButtonBase code review

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

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

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: 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

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: ButtonBase code review

Olivier Terral
In reply to this post by Cameron Shorter
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.
 

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev