Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/event CLIP_SCROLLED #243

Closed
wants to merge 7 commits into from
Closed

Feature/event CLIP_SCROLLED #243

wants to merge 7 commits into from

Conversation

adreyfus
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 3.864% when pulling 41afd03 on adreyfus:feature/event_CornerstoneClipScroll into 3bddb49 on chafey:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 3.865% when pulling 3e4a318 on adreyfus:feature/event_CornerstoneClipScroll into 0545a24 on chafey:master.

@swederik
Copy link
Member

swederik commented Nov 9, 2017

What do you plan to do with this event? Does CornerstoneNewImage not suffice?

@swederik swederik added the Change: Implementation 💻 Change that updates implementation details w/o expanding the API label Nov 9, 2017
@adreyfus
Copy link
Member Author

Hi,

'CornerstoneClipScroll' is the equivalent event to 'CornerstoneStackScroll' for clips. I have a synchronizer on them.

The 'CornerstoneNewImage' event is triggered at each calling of cornerstone.displayImage, and does not contain the direction information.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 3.8% when pulling 5124bb1 on adreyfus:feature/event_CornerstoneClipScroll into c6f12bf on chafey:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 3.8% when pulling cce85a6 on adreyfus:feature/event_CornerstoneClipScroll into c6f12bf on chafey:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.001%) to 4.269% when pulling 7ae6d23 on adreyfus:feature/event_CornerstoneClipScroll into 3e4a2d2 on cornerstonejs:master.

@adreyfus adreyfus changed the title Feature/event cornerstoneClipScroll Feature/event CLIP_SCROLLED Jan 17, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 4.267% when pulling 5d1d222 on adreyfus:feature/event_CornerstoneClipScroll into 3e4a2d2 on cornerstonejs:master.

@dannyrb
Copy link
Member

dannyrb commented Mar 9, 2018

The 'CornerstoneNewImage' event is triggered at each calling of cornerstone.displayImage, and does not contain the direction information.

Would you be able to calculate direction by:

  • Tracking previous imageId displayed
  • Getting the new imageId emitted on cornerstone.displayImage
  • Find both imageId's index in stack
  • Compare indexes to determine direction?

I'm not against adding this, and if you added tests, it would probably be fine to. I just want to make sure we're not emitting data multiple times when an image changes without good reason.

@adreyfus
Copy link
Member Author

Hi,

The solution you propose seems more complex. This PR is deliberately small with few changes, we simply add useful information.

@adreyfus
Copy link
Member Author

I don't know how to test features that require a complete cornerstone environment.

@dannyrb
Copy link
Member

dannyrb commented Nov 23, 2018

It has been several months since this has been touched. In the interest of keeping things clean, I am going to close this for now. I'm not sure this adds enough value to increase our external API. I'm also not convinced that this is the best way to expose this information.

It may be a better fit to determine and add direction to a cornerstone core event?

I appreciate your contributions! If you feel this is worth discussing, I am more than willing to work with you to try and identify a way to merge similar functionality.

@dannyrb dannyrb closed this Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change: Implementation 💻 Change that updates implementation details w/o expanding the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants