GUI: Any checkbox in standard table does not set correctly after sort

[davidkeymer]

Example:

After sorting a column in the Blocks tab of the Edit Configuration and Edit Component dialogue boxes, when selecting a "visibility checkbox", other checkboxes are randomly selected. This makes it impossible to configure the blocks as intended.

For a good example to demonstrate the behaviour, see the Lakeshore218 component on TOSCA (where the bug was first noticed).

Notes

1. It doesn't seem to matter which column has been sorted.
2. It doesn't happen when a column hasn't been sorted.
3. This behaviour occurs elsewhere in the GUI where there is a check box.

[John-Holt-Tessella]

I think this is every checkbox that uses /uk.ac.stfc.isis.ibex.ui.configserver/src/uk/ac/stfc/isis/ibex/ui/configserver/CheckboxLabelProvider.java

line 61 and 53 seem to return the wrong check box when called after sort. Also it seems to be triggered 3 times.

[Alexandru-Dascalu]

The problem was that we were adding a new listener every time the check box was ticked or unticked. After sorting, the models where moved around, but the checkboxes stay in the same row. Therefore, a checkbox still had listeners referring to models that no longer corresponded to it.

I also commited automatically generated changes to the danfysik opi, since I was told these were changes to ensure the opi is valid.

I have also changed the cellControls map in ControlCellLabelProvider so that the keys are the models stored in the table, not viewer cells. This is because in the refresh method of ViewerColumn, where the update method of the label provider is called, the Javadoc says explicitly not to cache viewer cells. And by looking through other methods in the Eclipse framework, we can see that there only one ViewerCell object ever, and is updated with new model information every time it is used, so storing it in a map makes no sense.

[Alexandru-Dascalu]

I have not manually tested that the bug has disappeared for IOC PvSets table in the edit IOC window, since I have been told that it is very rarely used and it did not have any data in the configs on my machine. Since the label provider subclass for that table does not have any logic relevant to the ticket, it is most likely the bug is fixed for that table as well.

[Kevin-Woods-Tessella]

@Alexandru-Dascalu - I don't think we should assume that bugs have disappeared, even if the feature is used only rarely. If we have spent time changing the code to fix a bug, we should spend the time testing that it really is fixed. If we require configs that contain appropriate data to test that the bug has been eliminated we should create the data. Is there an automated test for this bug? If so, what data does it use? If there is no automated test for this bug, why not?

[John-Holt-Tessella]

@Kevin-Woods-Tessella the pv sets feature is not in opperation, I believe that this can only be set for the galil and there is only a single checkbox which can be sorted the user manual says (https://github.com/ISISComputingGroup/ibex_user_manual/wiki/Create-And-Manage-Configurations#ioc-pv-sets):

IOC PV Sets is an experimental feature within IBEX do not use this before talking to the IBEX team. >It can be used to load in autosaved values from a specific file setup beforehand.

I wonder whether we should remove it from the gui.

[Alexandru-Dascalu]

@Kevin-Woods-Tessella Initially I thought the same but John advised it is not that important. Regarding automatic testing, me and Tom are in the process of making a squish test, though the squish will be for blocks table.

[KathrynBaker]

Best practice for testing is to test all routes to, through, and from the code – if for any reason these tables diverge over time then we need the tests to either break and show that loss of functionality, or pass and things not need to be looked at. Sorry John, I at least disagree, and it is probably a good thing I’m not likely to review this ticket as that would probably be a rework if all uses of the table and behaviour weren’t considered.

We labelled PVSets experimental because the functionality wasn’t well documented or understood – that is not the same as not needing it. Whilst it is there, we should test things like this within it, and deal with whether or not it should be there as a separate stream of work.

[Alexandru-Dascalu]

Pull requests:

https://github.com/ISISComputingGroup/System_Tests_UI_E4/pull/34 https://github.com/ISISComputingGroup/ibex_gui/pull/1050