cornerstoneTools
cornerstoneTools copied to clipboard
fix(GlobalToolChangeHistory): implemented a Map data structure to prevent duplication of change history logs
- Please check if the PR fulfills these requirements
- [x] The commit message follows our guidelines
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been added / updated (for bug fixes / features)
-
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) This PR contains a bug fix.
-
What is the current behavior? (You can also link to an open issue here) The current behavior is that when I make the activation of a tool repeated times, it floods the
globalToolChangeHistory
and then default enabled tools don't get enabled back when I switch to a new study. I've produced an example of the behavior with the following steps, first I would only use Cornerstone and CsTool and open a simple study, thus, some values would be be added to theglobalToolChangeHistory
by default as the following:
cornerstoneTools.store.state.globalToolChangeHistory = [
{
"mode": "active",
"args": [
"Pan",
null
]
},
{
"mode": "passive",
"args": [
"Length",
null
]
},
// [ . . . ]
{
"mode": "active",
"args": [
"StackScrollMouseWheel",
{
"isMouseWheelActive": true,
"mouseButtonMask": []
}
]
},
// [ . . . ]
]
So we can see that, only by opening a study, we have a bunch of fulfilled information about the globalToolChangeHistory
, the problem that I've observed was with the StackScrollMouseWheel
tool, but I think that it could happen with other tools that may have a similar behavior, specially custom tools.
The thing is that, since the globalToolChangeHistory
state is implemented as an array, the updates of Tool Mode are pushed indiscriminately to the array, and the "oldest" change is thrown away once the array reaches a limit of 50 entries.
What this means is that I could have a situation where I would activate and deactivate tools over and over and this would flood my globalToolChangeHistory
, for example, after opening my study I started to switch over the tools Zoom, Pan, Wwwc and Spatial Locator, and the state got like this:
cornerstoneTools.store.state.globalToolChangeHistory = [
{
"mode": "active",
"args": [
"StackScroll",
{
"mouseButtonMask": [
1
],
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"SpatialLocator",
{
"mouseButtonMask": [
1
],
"synchronizationContext": {
"enabled": true
},
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"Zoom",
{
"mouseButtonMask": [
1,
2
],
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"Pan",
{
"mouseButtonMask": [
1,
4
],
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"SpatialLocator",
{
"mouseButtonMask": [
1
],
"synchronizationContext": {
"enabled": true
},
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"Wwwc",
{
"mouseButtonMask": [
1
],
"isMouseActive": true,
"isTouchActive": true
}
]
},
{
"mode": "active",
"args": [
"Pan",
{
"mouseButtonMask": [
1,
4
],
"isMouseActive": true,
"isTouchActive": true
}
]
},
// [ . . . ]
]
Although I removed a great part of the 50 entries that are necessary to fill out the globalToolChangeHistory
, we can see that it is flooded by the actions I've made, and with objects that are literally identical.
The important part here, is to note that the entry for the StackScrollMouseWheel
is not present anymore, and once I change the study and, eventually, the _repeatGlobalToolHistory
function gets called it will try to repeat the mode change history, but it will not contain the activation of the StackScrollMouseWheel
tool, and it will be kept disabled, causing the image stack to not be scrollable by the MouseWheel
event.
-
What is the new behavior (if this is a feature change)?
So the general ideal was to switch the data structure used for the
globalToolChangeHistory
from a JavaScript Array to a JavaScript Map. Using that strategy we would avoid the problem of flooding the history with repeated entries that are not relevant due to duplications, so only the most updated information about a specific tool would be stored on the history. Plus, since duplications of those updates aren't possible anymore, I could not find a way to fill out the 50 entries completely although I kept the limit as it was.
Given the same steps as described above, I would get a default globalToolChangeHistory
as:
cornerstoneTools.store.state.globalToolChangeHistory = new Map([
[
"Length",
{
"mode": "passive",
"args": [
"Length",
null
]
}
],
[
"Pan",
{
"mode": "active",
"args": [
"Pan",
{
"mouseButtonMask": [
4
],
"isMouseActive": true,
"isTouchActive": true
}
]
}
],
[
"StackScrollMouseWheel",
{
"mode": "active",
"args": [
"StackScrollMouseWheel",
{
"isMouseWheelActive": true,
"mouseButtonMask": []
}
]
}
],
// [ . . . ]
])
And after reproducing the same steps as before repeated times, I would have the exact same number of entries, although that the values could be changed. I have some custom tools and they added some entries when used, but it wasn't near of the 50 entries upper limit.
After the changes, when I do the steps described and switch to a new study, the StackScrollMouseWheel
is working fine, identifying the MouseWheel
events and working as I would expect.
-
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) This PR shoudn't introduce any breaking change.
-
Other information: The commit 5d4fd23569738873c0517035a8083311f4526f78 was added with the intention of removing a line of code that could be conditionally ran and could lead to a function error, this because there was a explicitly defined object that had a conditional call to the method
push
that would throw an error if called. This change was made onsrc/store/setToolMode.js
at lines 531-533.
Another useful information is that, the change from an simple Array
to a Map
could introduce some concern on what relates to the browsers being able to use it. So I would like to link here the page of Can I use Map()? that reports a 97.22% of Global support, and 99.65% for Desktop users.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
dbf874f
) 23.86% compared to head (5d4fd23
) 24.23%.
Additional details and impacted files
@@ Coverage Diff @@
## master #1565 +/- ##
==========================================
+ Coverage 23.86% 24.23% +0.37%
==========================================
Files 287 287
Lines 10141 10141
Branches 2082 2081 -1
==========================================
+ Hits 2420 2458 +38
+ Misses 6575 6549 -26
+ Partials 1146 1134 -12
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.