Kitware / itk-vtk-viewer

2D / 3D web image, mesh, and point set viewer using itk-wasm and vtk.js
https://kitware.github.io/itk-vtk-viewer/
BSD 3-Clause "New" or "Revised" License
210 stars 64 forks source link

Rework event subscriptions #289

Closed scottwittenburg closed 4 years ago

scottwittenburg commented 4 years ago

@thewtex I still have to finish changing the many subscribe methods for backwards compatibility, but this gives you an idea where it's headed. I plan to wrap it up tomorrow, so maybe take a look and let me know what you think.

Also, I added a publicAPI.setLabelMap method. Not sure how to test it at the moment.

@oeway fyi

scottwittenburg commented 4 years ago

p.s. the WIP commit to dist/index.html is something I'll remove, I'm just using it to test the event system, and so you can see how it works or try it out if you want.

thewtex commented 4 years ago

@scottwittenburg looking great! Can we bump the major API version and remove the subscribe* functions from pubicAPI?

scottwittenburg commented 4 years ago

@thewtex I'm also adding the imagePicked event as we speak, just fyi, and I'll close #284 when I have it working in here.

@scottwittenburg looking great! Can we bump the major API version

Will do.

and remove the subscribe* functions from pubicAPI?

I left those in there for two reasons:

  1. backwards compatibility
  2. The eventemitter3 subscription functions do not return the listener for easy unsubscribe, and these subscribe methods provide that feature.

If you'd still rather have them removed, I'll do it.

oeway commented 4 years ago

Looks good! Will you also expose the eventEmitter as a public api for embedded applications? I think that will be great.

scottwittenburg commented 4 years ago

Looks good! Will you also expose the eventEmitter as a public api for embedded applications? I think that will be great.

Sure, something like publicAPI.getEventEmitter = () => eventEmitter?

thewtex commented 4 years ago

backwards compatibility

Good to think about, but at this point, I think it is better to keep moving forward with a clean API.

The eventemitter3 subscription functions do not return the listener for easy unsubscribe, and these subscribe methods provide that feature.

It looks like off or removeListener can be used, though? We have not been unsubscribing, so it does not seem to be crucial.

scottwittenburg commented 4 years ago

backwards compatibility

Good to think about, but at this point, I think it is better to keep moving forward with a clean API.

👍

The eventemitter3 subscription functions do not return the listener for easy unsubscribe, and these subscribe methods provide that feature.

It looks like off or removeListener can be used, though? We have not been unsubscribing, so it does not seem to be crucial.

That's correct, you have those for unsubscribe. You just need to replace any anonymous handlers with named ones for off and removeListener to work. But that's no big deal. I'll go ahead and remove all the subscribe<event-name> methods. Thanks!

scottwittenburg commented 4 years ago

@thewtex I think this is only missing the renaming you requested, which I'm working on now.

thewtex commented 4 years ago

@scottwittenburg hurrah! Looks great!

thewtex commented 4 years ago

:tada: This PR is included in version 10.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: