cornerstoneTools icon indicating copy to clipboard operation
cornerstoneTools copied to clipboard

fix(GlobalToolChangeHistory): implemented a Map data structure to prevent duplication of change history logs

Open ViniciusResende opened this issue 1 year ago • 1 comments

  • 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 the globalToolChangeHistory 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 on src/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.

ViniciusResende avatar Feb 09 '24 16:02 ViniciusResende

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.

codecov[bot] avatar Feb 09 '24 17:02 codecov[bot]