cornerstonejs / cornerstoneTools

A framework for tools built on top of Cornerstone.
https://tools.cornerstonejs.org/
MIT License
574 stars 456 forks source link

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

Closed ViniciusResende closed 2 months ago

ViniciusResende commented 4 months ago
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.

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.

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()?](https://caniuse.com/?search=Map()) that reports a 97.22% of Global support, and 99.65% for Desktop users.

codecov[bot] commented 4 months ago

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 ```diff @@ 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.