[OpenLayers-Trac] Re: [OpenLayers] #3016: ModifyFeature - "del" key press outside vertice should delete the feature

[OpenLayers]

#3016: ModifyFeature - "del" key press outside vertice should delete the feature
-----------------------------------+----------------------------------------
 Reporter:  adube                  |       Owner:  tschaub     
     Type:  feature                |      Status:  new         
 Priority:  minor                  |   Milestone:  2.11 Release
Component:  Control.ModifyFeature  |     Version:  2.10        
 Keywords:  del, delete, key       |       State:  Review      
-----------------------------------+----------------------------------------

Comment(by erilem):

 Replying to [comment:10 adube]:
 > I modified and added the required tests and added some new features :
 >
 >  * setState method in OpenLayers.Feature.Vector to actually set the
 state without validating anything and trigger the OpenLayers.Layer.Vector
 "featurestatechanged" event.
 >  * toState method modified to do the validation and call setState
 >
 > I checked the Layer/Vector.js, Feature/Vector.js and
 Control/ModifyFeature.js tests in FF 3.6, 4 and IE8 and everything seemed
 fine.  I got an error in FF 4 but it was not related to the modifications.

 Thanks a lot for the patch. Some review comments below.

 tests/Layer/Vector.html[[BR]]
 tests/Control/ModifyFeature.html

  * we often rely on a {{{log}}} object to test that a function gets
 called, so  message is displayed even if the function is not called. E.g.
  {{{
 log = false;
 function expected() {
     log = true;
 }
 t.ok(log, "expected function called");
  }}}

 tests/Control/ModifyFeature.html

  * I'd rather have a separate test function for "delete key pressed while
 on feature but outside a vertex"

 lib/OpenLayers/Control/ModifyFeature.js

  * we need to document the new behavior in the {{{ModifyFeature}}} API
 docs somewhere

 lib/OpenLayers/Feature/Vector.js

  * if we want to rely on {{{prevState}}} as done in your patch we should
 properly document it
  * but I'd rather have the {{{featurestatechanged}}} event include the
 previous and new states ({{{{feature: this, state: state, previous:
 prevState}}}})

 examples/delete-with-modify-feature.html

  * having a specific example is fine I think, but I'd use a simpler name,
 {{{delete-feature.html}}}

-- 
Ticket URL: <http://trac.openlayers.org/ticket/3016#comment:12>
OpenLayers <http://openlayers.org/>
A free AJAX map viewer