OpenLayers OpenLayers

Ticket #1490 (closed bug: fixed)

Opened 8 months ago

Last modified 7 months ago

feature.createPopup fails after popup.destroy

Reported by: pspencer Assigned to:
Priority: minor Milestone: 2.7 Release
Component: documentation Version: SVN
Keywords: Cc:
State: Review

Description

If you create a Feature then use that Feature to createPopup, then destroy the popup and then call createPopup again, a new popup is not created because Feature.popup is not cleared by the destroy. Wow, that sounds complicated! But really, it comes down to unsetting the popup attribute of the feature when the popup is destroyed.

Patch attached, not sure if this is the right fix but it works.

Attachments

patch.diff (447 bytes) - added by pspencer on 04/03/08 17:09:05.
feature popup patch

Change History

04/03/08 17:09:05 changed by pspencer

  • attachment patch.diff added.

feature popup patch

04/03/08 17:09:42 changed by pspencer

  • state set to Review.

04/03/08 17:22:12 changed by crschmidt

I think this is:

  • Not a regression
  • A bad way to use the code, because you didn't create the popup: you destroying it seems destined for failure at some point.

This is no more clearly a 2.6 RC candidate than #1461, in my eyes.

04/05/08 05:50:20 changed by crschmidt

  • owner deleted.
  • version changed from 2.6 RC1 to SVN.
  • component changed from Popup to Feature.
  • milestone changed from 2.6 Release to 2.7 Release.

Even more so than this being the wrong way to use the code, I just don't like the fix. Popups aren't aware of features. There's no reason for them to be, so I don't like this connection. I'm bumping to 2.7, unless there's some behavior here that changed in 2.6 from pre-2.6, but I don't see anything obvious.

The 'right' way to do this is to

  1. Figure out why you need to destroy: are you just trying to hide the popup, perhaps?
  2. If you need it, add a feature.destroyPopup() method.

04/07/08 21:55:23 changed by pspencer

  • owner changed.
  • priority changed from major to minor.
  • component changed from Feature to documentation.

Chris, the correct behaviour in our case is to just hide the popup. I agree that popups are not aware of features.

We (and probably others) have many popups in this particular app and so a potential memory management problem keeping all the popups around.

Since createPopup returns something, it seems likely that someone will look up the documentation for Popup and try destroying it, thinking that is the right thing to do.

I think we should add something to the docs in createPopup to say that the caller should *not* destroy the popup and add a feature.destroyPopup method. Um, there is already a destroyPopup method ... forget I said that, I suggest that this becomes a documentation bug to update createPopup method with a note that the caller should destroy the popup using destroyPopup.

04/07/08 22:06:36 changed by pspencer

Another question - would changing the popup's destroy function in feature.createPopup to call feature.destroyPopup be a bad thing?

this.popup.destroy = OpenLayers.Function.bind(this.destroyPopup, this);

05/13/08 16:19:55 changed by elemoine

  • status changed from new to closed.
  • resolution set to fixed.

This ticket should have been closed by r6820.