CSSTUDIO-1950 Remove calls to fireItemDataConfigChanged() when archivers are removed from instances of PVItem.

[abrahamwolk]

This merge-request removes calls to fireItemDataConfigChanged() when archivers are removed from instances of PVItem.

I am not sure this fix is a "correct" fix or not, and therefore I would like to ask about your opinion on this.

The intention with the merge-request is to fix an issue that occurs when some configured archivers don't archive a specific PV: if a Data Browser configuration is loaded that contains a PV that some archivers don't archive, then those archivers will automatically be removed from the PVItem instance associated with the PV in question by calling removeArchiveDataSource(). The function removeArchiveDataSource(), in turn, calls the function fireDataItemChanged(), which, in turn, (at least when I reproduce the issue) calls 7 different instances of ModelListeners.changedItemDataConfig(). One of the invoked instances is DataBrowserInstance.changedItemDataConfig(), which, in turn, calls setDirty(true).

The result is that about 1 second or so after loading the Data Browser configuration, the dirty mark is set even though no change in the Data Browser configuration has occurred.

By removing the call to fireItemDataConfigChanged() in removeArchiveDataSource(), the dirty mark is not set and the issue doesn't occur. However, a consequence is that the 6 instances of ModelListeners.changedItemDataConfig() other than DataBrowserInstance.changedItemDataConfig() are also not invoked. When I reproduce the issue, the instances in question are:

1. TracesTab.changedItemDataConfig()
2. TimeAxisTab.changedItemDataConfig()
3. AxesTab.changedItemDataConfig()
4. MiscTab.changedItemDataConfig()
5. StatisticsTabController.changedItemDataConfig()
6. Controller.changedItemDataConfig()

Do these functions need to be invoked when an archiver is removed from a PV?

[kasemir]

There's a related setting dropped_failed_archives,
https://github.com/ControlSystemStudio/phoebus/blob/f6ace8301142047e207bbf435decdb68b379594e/app/databrowser/src/main/resources/databrowser_preferences.properties#L93C10-L93C10

The default is true because that's usually more efficient: If an archive data source has an error, don't keep calling it in vain. But it can also mean that once you have a temporary network issue, those data sources are removed and you never get the data even after the source is again online.

The 'dirty' state is somewhat troublesome beyond the auto-dropped data sources. Open a plot file, zoom in or out as one does when looking around, and now the config is considered 'dirty'. For operational displays, it's usually best to once save them with "Misc.", "Save Changes [ ]" un-checked. That way, users can open them and do whatever they want, it stays 'clean' and will open in the same original way when opened again, not trying to save any changes.
Maybe we make that a little more convenient by automatically un-checking "Save Changes" when the file is read-only, so just copying a file into a production location that's read-only to mere mortals we no longer bother users with "Do you want to save ...".

Simply removing the fireItemDataConfigChanged calls as you propose will likely be OK. It does mean that if you actually tweak the data sources, you'll have a hard time saving those changes. In the olden days when we had more than 50(!) "Channel Archiver" data sources, then transitioned to the RDB setup, .. it was important to control the data sources for each channel.
Nowadays most people have just one data source (one RDB or one master Archive Appliance access point). You can set use_default_archives=true and end users never need to fiddle with the "sources" section of a trace.

So bottom line I'd say go ahead.

[abrahamwolk]

Thank you for your detailed answer. I indeed have use_default_archives=true set, and because of this there was no change in the saved .plt-file after archivers had automatically been removed from a PVItem after they reported that the PV in question wasn't found.

I have now updated the merge-request so that fireItemDataConfigChanged() is invoked only when use_default_archives=false, since then it really reflects a file change.

[kasemir]

updated the merge-request so that fireItemDataConfigChanged() is invoked only when use_default_archives=false, since then it really reflects a file change.

That's a great idea!