Proper way to dispose editor window subscriptions?


#1

I have the following code …

  activate: ->
    @subs = new SubAtom
    @subs.add atom.workspace.observeTextEditors (editor) =>
      lines = atom.views.getView(editor).shadowRoot.querySelector '.lines'
      @subs.add lines,     'mousedown', (e) => @mousedown e, editor, lines

As you can see, I’m adding a listener to every editor. But what happens to my subscription when an editor is destroyed? The disposable in my list will still be attached to the destroyed editor. I assume the editor can’t dispose my subscription.

I could watch for editor destroy events but that would be a real pain. I would have to store the disposables in a way that they could be indexed by the editor but compositeDisposables (sp?) isn’t set up for that, right?

What does everyone else do?

Edit: I guess this applies to any destroyable object but I don’t remember running into this before.


#2

Often I try to only subscribe to the active editor. Any time the active editor changes, I dispose of old subscriptions and subscribe to the new active editor. This way, I never have to worry about when an editor is destroyed.

If that strategy doesn’t work, then yes, to properly clean up things you should unsubscribe from editor events when the editor is destroyed. Or at least release the CompositeDisposable holding the subscriptions for that editor.


#3

Thanks.

This is probably just a terminology thing but what you do you mean by “release”. Dispose?


#4

I mean release as in null out the reference to it, i.e.:

disposable = null

#5

In this situation I would do like this:

activate: ->
    @editorsSubscription = atom.workspace.observeTextEditors (editor) =>
      subs = new SubAtom # or CompositeDisposable
      lines = atom.views.getView(editor).shadowRoot.querySelector '.lines'
      subs.add lines, 'mousedown', (e) => @mousedown e, editor, lines
      subs.add editor.onDidDestroy -> subs.dispose()

Keeping only a subscription in the instance for the observer and then a CompositeDisposable in the block that holds the subscriptions for the editor and that can be disposed without affecting other subscriptions.


#6

This will fail to dispose the mousedown and destroy events when the package is deactivated. I usually create a new CompositeDisposable and add/remove it from the main one as needed.

activate: ->
    @subscriptions = new SubAtom
    @subscriptions.add atom.workspace.observeTextEditors (editor) =>
      editorSubscriptions = new SubAtom
      lines = atom.views.getView(editor).shadowRoot.querySelector '.lines'
      editorSubscriptions.add lines, 'mousedown', (e) => @mousedown e, editor, lines
      editorSubscriptions.add editor.onDidDestroy ->
        editorSubscriptions.dispose()
        @subscriptions.remove(editorSubscriptions)
      @subscriptions.add(editorSubscriptions)
deactivate: ->
    @subscriptions.dispose()

#7

It’s crazy to have to do so much for such a simple common operation. I’m going to investigate adding some feature to SubAtom to make this easy.


#9

I’ve added the feature to sub-atom and I’m using it in the code I asked the question about. Thanks @postcasio for the algorithm. This feature collapsed six lines into one line in the sample below. Here is the relevant section of the readme …


Auto-Dispose On Event

When you attach a subscription to an element, you need to dispose the subscription when the element is destroyed instead of waiting until the end when everything is disposed. This is complex and often not handled properly. Atom-sub makes this easy.

This is an example of the normal method to attach a click subscription to a paneView and dispose the subscription when the pane is destroyed …

paneSubs = new SubAtom
paneSubs.add paneView, 'click', => @click pane
paneSubs.add pane.onDidDestroy ->
  paneSubs.dispose()
  @subs.remove paneSubs
@subs.add paneSubs

This is the sub-atom version. Notice that it replaces the six lines with one line and is more readable.

@subs.add paneView, 'click', pane, 'onDidDestroy', => @click pane

This feature is enabled by adding the tigger event to the add function as two arguments, the object and the name of the method on that object. The new signatures are …

subs.add disposable, eventObject, eventName
subs.add target, events, [selector], eventObject, eventName, handler

#10

Have you considered using an interface that takes an object so that all the parameters are effectively named? In isolation it’s hard to tell what all those options are. Using destructuring the only change to your function is to wrap the parameters in { }.

@subs.add paneView, 'click', pane, 'onDidDestroy', => @click pane

@subs.add {
    target: paneView, 
    events: 'click', 
    eventObject: pane, 
    // eventName doesn't tell what's happening. I'd probably call this `destroyOn`
    // Should this just be a default behavior?
    eventName: 'onDidDestroy', 
    handler: => @click pane
}

#11

Taking an object didn’t make sense when it was just copying CompositeDisposable and jQuery.on. They are both burned into my brain and I assumed other’s brains. So I definitely want to support the original two signatures.

However, I seen no reason why it couldn’t support calling with an object. It could just check to see if the first arg contains any of the key property names.

It is designed to work with any event. Maybe some don’t use that name. I also thought there may be other events than destroy that this could be useful for. I haven’t given it much thought. Maybe it will always be a destroy.

Just tell what me property names and behavior you want for the object-type call and I’ll happily implement it. I’m flexible.


#12

BTW, a minor point. Coffeescript doesn’t require {} or commas. This would work …

@subs.add
    target: paneView
    events: 'click'
    eventObject: pane
    eventName: 'onDidDestroy'
    handler: => @click pane

Edit: Oh, I see you meant destructuring with {}. Yes they are needed then.