Skip to content

Improve robustness of alarm tree update logic - #3987

Open
LordIdra wants to merge 1 commit into
ControlSystemStudio:masterfrom
LordIdra:technical-alarm-tree-fixes
Open

LordIdra wants to merge 1 commit into
ControlSystemStudio:masterfrom
LordIdra:technical-alarm-tree-fixes

Conversation

@LordIdra

Copy link
Copy Markdown

While trying to implement a search bar for the alarm tree, I ran into a few issues:

  • Updates to the model were not processed on the UI thread.
    • This means that the UI was being modified from the update processing thread, which caused me quite a headache when I got intermittent random errors in the update handling code while implementing the search function. I eventually traced it to this
    • I think this was a simple typo/oversight which hasn't caused issues until now
  • The items_to_update map held references to items in the UI.
    • These were obtained on a separate thread when an update came in, instead of on the UI thread, which could cause a variety of issues e.g. if an item is removed and then re-added between updates.
    • I have changed this so that it stores the paths to the view items and then skips updating them if they are not present when the updates are processed on the UI thread
  • itemAdded and itemRemoved obtained the view item's parent on a separate thread, and then re-used that in code running on the UI thread later, same issues as above.
    • I've just changed this so that it all runs on the UI thread
  • removed unused updateStats stuff while I was at it

tl;dr: made stuff which touches the UI run on the UI thread

Checklist

  • Testing:

    • The feature has automated tests
    • Tests were run
    • If not, explain how you tested your changes
  • Documentation:

    • The feature is documented
    • The documentation is up to date
    • Release notes:
      • Added an entry if the change is breaking or significant
      • Added an entry when adding a new feature

@sonarqubecloud

Copy link
Copy Markdown

@LordIdra LordIdra mentioned this pull request Oct 27, 2026
2 of 6 tasks
@shroffk

shroffk commented Oct 27, 2026

Copy link
Copy Markdown
Member

I believe this was by design...
When all changes to the model were happening on the UI thread you had each node and pv addition/removal result in a refresh which, this made lading of the tree extremely slow and confusing....particularly for large alarm trees. Hence the decision to make changes off the UI thread and then having them rendered.

I am sure there are some improvements from here we should be able to adopt but this could need a bit more consideration.

@LordIdra

Copy link
Copy Markdown
Author

I believe this was by design... When all changes to the model were happening on the UI thread you had each node and pv addition/removal result in a refresh which, this made lading of the tree extremely slow and confusing....particularly for large alarm trees. Hence the decision to make changes off the UI thread and then having them rendered.

I am sure there are some improvements from here we should be able to adopt but this could need a bit more consideration.

Maybe my initial explanation was a little unclear (or I am misunderstanding?) - all the model logic (ie stuff that modifies AlarmTreeItem<?>) still occurs off of the UI thread. The changes here only brings a little bit more code into the UI rendering so that all the code which interacts with the UI is on the same thread at the same time, since previously there was a split which introduced race conditions.

Putting aside the initial startup code, the itemAdded/itemRemoved code does block on the UI thread anyway (so the add/remove rate is effectively capped to the framerate). From the history and comments I thought that this was intended as the solution to the 'trees loading weirdly' problem?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants