fix: Replace explicit calls to refreshToolboxSelection with a workspace listener #8980
Conversation
|
Drive-by review: I don't love adding timeouts without a clear understanding of why they're needed. It seems like we didn't need them when the call was in workspace, so I don't understand what changed. My guess is it's needed due to the delay on events firing (though I don't know why that didn't apply before). If so, I wonder if a more straightforward approach would be to register an event listener for the various variable events that calls the |
|
Switching to the event listener seems to be working correctly and allowed me to remove additional calls to refreshToolboxSelection. Please double check my use of internal as I wasn't sure what scope the new callback function should have. |
|
Rather than adding this to |
|
@gonfunko Just tried that and it looks like the variable change events only get fired on the main workspace, not the flyout workspace, so I'd have to register a separate listener either way. It might be slightly more efficient to register a separate listener on the target listener in flyout.show and unregister it in hide, but it's also a bit more state to manage if I go that route. What do you think? |
The basics
The details
Resolves
Fixes #8975
Proposed Changes
Wraps the refreshToolbox calls in a setTimeout so they happen after the changes to variables finish.
Reason for Changes
We were ending up in an infinite recursion as the toolbox refreshed before the variables were fully created which then tried to create the variables again.
Test Coverage
Will need a follow-up PR to add tests for this.
Documentation
Additional Information