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

feat(imageIdSpecificStateManager.js): Methods for managing toolState via imageId #1151

Merged

Conversation

mikehazell
Copy link
Contributor

Expose [get|add|clear]ImageIdToolstate methods which allow managing imageId specific toolState without needing an element with the image loaded.

#1150

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

New feature

  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

3 new methods on the ImageIdSpecificToolStateManager for managing toolState without needing an element.

    getImageIdToolState,
    addImageIdToolState,
    clearImageIdToolState,

The signature for these matches that of the exising add, get, clear methods except that the first argument is an imageId instead of an element.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No

  • Other information:

…lState via imageId

Expose [get|add|clear]ImageIdToolstate methods which allow managing imageId specific toolState
without needing an element with the image loaded.

cornerstonejs#1150
@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #1151 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1151   +/-   ##
=======================================
  Coverage   18.95%   18.95%           
=======================================
  Files         282      282           
  Lines        8422     8422           
  Branches     1434     1434           
=======================================
  Hits         1596     1596           
  Misses       5658     5658           
  Partials     1168     1168
Impacted Files Coverage Δ
src/util/freehand/pointInFreehand.js 60.71% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5473465...84dfb3e. Read the comment docs.

@dannyrb
Copy link
Member

dannyrb commented Jan 21, 2020

This makes a lot of sense to me. I've witnessed too many applications manipulating state directly by drilling down. This feels much safer and less in the weeds. If we want to swap managers, the ID agnostic fields still satisfy the interface -- any state manager not scoped to ID wouldn't use the new methods either.

@dannyrb dannyrb merged commit eac36e6 into cornerstonejs:master Jan 21, 2020
@dannyrb
Copy link
Member

dannyrb commented Jan 21, 2020

🎉 This PR is included in version 4.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehazell mikehazell deleted the pr/add-get-imageIdSpecificToolState branch January 22, 2020 01:48
Comment on lines +98 to +105
function clearElementToolState(element) {
const enabledElement = external.cornerstone.getEnabledElement(element);

if (
!enabledElement.image ||
toolState.hasOwnProperty(enabledElement.image.imageId) === false
) {
if (!enabledElement.image) {
return;
}
clearImageIdToolState(enabledElement.imageId);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was an error, missing the "image" object

clearImageIdToolState(enabledElement.imageId);

to change:

clearImageIdToolState(enabledElement.image.imageId);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants