Ticket #1508 (reopened task)

Opened 5 years ago

Last modified 3 years ago

*featureadded* event for DrawFeature control

Reported by: sbenthall Owned by: tschaub
Priority: critical Milestone: 3.0 Release
Component: Control Version: 2.6 RC2
Keywords: Cc:
State: Needs Discussion

Description

The DrawFeature control has a featureAdded callback that kicks in whenever a feature is added. But this method of providing callback behavior has been superseded by new event handling features.

The featureAdded callback should be replaced by a *featureadded* event triggered when the control draws a feature.

The reason why the Vector layer's *featureadded* behavior is not sufficient for this is that that event is triggered in much more general circumstances.

Attachments

DrawFeature.js.patch Download (1.4 KB) - added by sbenthall 5 years ago.
t1508.patch Download (3.3 KB) - added by tschaub 5 years ago.
featureadded with a couple tests

Change History

Changed 5 years ago by sbenthall

Changed 5 years ago by crschmidt

  • milestone set to 2.7 Release

Changed 5 years ago by crschmidt

  • owner set to sbenthall
  • priority changed from minor to critical

Changed 5 years ago by sbenthall

  • status changed from new to assigned

Changed 5 years ago by sbenthall

  • owner changed from sbenthall to tschaub
  • status changed from assigned to new
  • state set to Review

Changed 5 years ago by tschaub

featureadded with a couple tests

Changed 5 years ago by tschaub

  • status changed from new to closed
  • state changed from Review to Complete
  • resolution set to fixed

In w/ r7601.

Adding a featureadded event to the draw feature control event types. First tests ever for the draw feature control. Thanks sbenthall for the patch.

Changed 5 years ago by elemoine

  • status changed from closed to reopened
  • resolution fixed deleted

Other feature-related events are available in Layer.Vector. Why would this one be implemented differently? I see no reason. My proposition is to rename it featuredrawn and move it into Layer.Vector.

Also, we should make the editing toolbar control use that event instead of relying on the overriding of the featureAdded method.

Changed 5 years ago by elemoine

  • state changed from Complete to Needs Discussion

Thinking more about it I'm not sure this a good thing that all feature-related events are in Layer.Vector. More specifically, I think that (a) the events featureselected and featureunselected should go in Control.SelectControl, and (b) the events beforefeaturemodified, featuremodified and afterfeaturemodified should go in Control.ModifyFeature. Not only is this better design, it's also fixing bugs! For example, the modify feature control listens on featureselected, which makes it receives all featureselected events occurring on the layer, not only those generated by its internal select feature control. I'm thinking about bugs reported in #1483 and on the mailing list these days. Thoughts?

Changed 5 years ago by tschaub

Yeah, I agree we've got problems here. I'll see if I can get sbenthall to comment since the issue was his.

It is worth considering that a reference to the control is not always available. However, in the case of a panel, the panel should probably be listening for events from its obligate controls and triggering its own events.

Changed 5 years ago by sbenthall

Personally, I feel pretty strongly that events on the controls are a good idea.

Consider the case of a new control developed by a third party that interacts with vector features in some way. This coding precedent would dictate that for them to write that in OL style, they would have to hack the events related to that control into the Vector layer, as opposed to just providing a new class that can sit outside of the library. The latter seems far cleaner to me.

Changed 5 years ago by tschaub

  • milestone changed from 2.7 Release to 2.8 Release

Eric, I've been away from this for a while. I'm inclined to say let's have this discussion with 3.0 in mind. If you (or anyone else) wants something different in 2.7, please change this back to 2.7. (BTW, I fully agree that controls should trigger events.)

Changed 4 years ago by crschmidt

  • milestone changed from 2.8 Release to 2.9 Release

tschaub says '3.0'. I don't know if that's neccesary, but given no other comments, bumping to that milestone for now. Pull it back to 2.9 if you think we should do it there.

Changed 4 years ago by crschmidt

  • milestone changed from 2.9 Release to 3.0 Release

(Darn trackpad finger memory)

Changed 3 years ago by adube

I'd like to work on this one. The latest idea was to go with "all events should be triggered by controls". Should this still be the case ? Anything new I should be aware of before I begin ?

Changed 3 years ago by tschaub

Sorry appear a flip-flopper here, but if someone wants more events from the controls in 2.x, this can be done now. Same with more events from the layer.

I don't think there is a consensus about how things should be in 3.0 (at least one that is described in this ticket).

Note: See TracTickets for help on using tickets.