[LordIdra]

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]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.7% Duplication on New Code

See analysis details on SonarQube Cloud

[shroffk]

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]

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?

[LordIdra]

@shroffk coming back to this I think I see where the confusion came from, 'Updates to the model were not processed on the UI thread' is quite confusing wording. What I meant was that some code which applies to the UI any updates to the model (which in turn of course are made by the alarm model thread) was not running on the UI thread. This led to various race conditions which only surfaced when I was implementing the search box.

Fixing this should not really affect anything performance-wise, nor does it change the behaviour in any tangible way (AFAIK).

I am not 100% sure about the path2view changes as this seems to be a remnant of the old model which has been left behind. There isn't really any reason for this map to be updated concurrently, and from the comments about it, it seems like it's meant to support concurrent calls to itemRemoved and itemAdded and itemUpdated - but as these methods actually do the UI work on the UI thread anyway, there is basically no reason to instantiate or remove UI elements on a different thread.

Regarding the performUpdates - this runs UI code off the UI thread, and it should definitely not be doing that. The reason I'm fairly confident this is unintentional is

1. itemRemoved and itemAdded drop to the UI thread,
2. have a look at this PR: https://github.com/ControlSystemStudio/phoebus/pull/2615/changes - this is what made performUpdates stop doing updates on the main thread, it definitely looks like an unintended change
3. the UpdateThrottle docs say 'Can be triggered from any thread. The throttled 'update' will typically transfer to the UI thread.'

But please let me know if you have questions :) I appreciate it is a bit of a gnarly change