Ticket5811 table bug fixing #1318

JamesKingWork · 2021-06-23T14:01:02Z

Description of work

Correct how we update actions in the script generator actions table so we don't lose and reapply focus
Fix some minor bugs discovered whilst playing around

Ticket

ISISComputingGroup/IBEX#5811

Acceptance criteria

To test:

Add lots of rows very quickly and then try editing and tabbing between cells whilst the script generator validates and calculates an estimated time for all the actions
Confirm that general actions still work such as changing script definitions, adding, removing, moving actions up and down, saving and reloading parameters

Unit tests

Give an overview of unit tests you have added or modified, if applicable. The aim is provide information to help the reviewer

System tests

Mention any automated tests or manual tests that you have added or modified, if applicable. The aim is provide information to help the reviewer

Documentation

Highlight and provide a link to any additions or changes to the documentation, if applicable. The aim is provide information to help the reviewer

Code Review

Is the code of an acceptable quality?
Do the changes function as described and is it robust?
Is there associated PR for the release notes?

Final Steps

Reviewer has also merged the release notes

…r than using setInput

…nd remove unused imports and remove author

JamesKingWork · 2021-06-23T14:22:23Z

...x.ui.scriptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ActionsViewTable.java

-					// Add new action if tab is pressed by user in the last cell of the table.
-					if (nextCell.getNeighbor(ViewerCell.BELOW, false).getElement() == null
-					        && (viewer.getTable().getColumnCount() - NON_EDITABLE_COLUMNS_ON_RIGHT == currentlyFocusedColumn)) {
+					Optional<ViewerCell> neighbour = Optional.ofNullable(nextCell.getNeighbor(ViewerCell.BELOW, false));


Fixes NullPointerException where the ViewerCell from BELOW was null as it doesn't exist (which also means there is no action below the current one. The || is a cut so the call to get() should be fine.

JamesKingWork · 2021-06-23T14:23:21Z

...x.ui.scriptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ActionsViewTable.java

 	/**
 	 * Sets focus of cell.
 	 * @param row row number of table
 	 * @param column column number of table
 	 */
 	public void setCellFocus(int row, int column) {
-		if (row >= 0) {
-			viewer.editElement(viewer.getElementAt(row), column);
+		var element = viewer.getElementAt(row);


Fixes NullPointerException where the element is null so editElement cannot handle it.

JamesKingWork · 2021-06-23T14:23:33Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

@@ -86,16 +82,6 @@
     */
    private static final Color CLEAR_COLOR = DISPLAY.getSystemColor(SWT.COLOR_WHITE);

-    /**
-     * A light orange to use when validity checks may be incorrect e.g. for when using an unsupported language.


Not used anywhere so removed

JamesKingWork · 2021-06-23T14:23:50Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

@@ -59,9 +58,6 @@

 /**
 * The ViewModel for the ScriptGeneratorView.
- * 
- * @author James King


Don't know when this was added, but certainly hasn't been just me so removed

JamesKingWork · 2021-06-23T14:24:25Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

@@ -324,7 +310,6 @@ private void displayThreadingError() {
    protected void addEmptyAction() {
    scriptGeneratorModel.addEmptyAction();
    // Make sure the table is updated with the new action before selecting it
-    actionChangeHandler(viewTable, btnGenerateScript, btnSaveParam, btnSaveParamAs, true);


The actionChangeHandler should only be called when the action change is propagated from below.

JamesKingWork · 2021-06-23T14:25:00Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

    DISPLAY.asyncExec(() -> {
-        viewTable.setCellFocus(insertionLocation, 0);
+        viewTable.setCellFocus(insertionLocation, ActionsViewTable.NON_EDITABLE_COLUMNS_ON_LEFT);


Set the cell focus to the first editable column

JamesKingWork · 2021-06-23T14:26:04Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

@@ -767,25 +746,6 @@ protected String getFirstNLinesOfInvalidityErrors(int i) {
     * @param viewTable The table in the view to update.
     */
    protected void updateValidityChecks(ActionsViewTable viewTable) {
-    List<ScriptGeneratorAction> actions = scriptGeneratorModel.getActions();


Moved into the update calls in the various columns as this seemed more in-keeping with the swt tables

LowriJenkins

Adding a large number of rows quickly, and then changing the script definition to one with global variables causes the script genderator to crash.

Other than that everything seems to look fine, a couple of comments on the code.

I doubt its just your ticket thats done it, but i was checking various buttons still work when its handling lots of new inputs in the table and it looks like theres a problem with the help button? it seems to link to an empty page.

LowriJenkins · 2021-07-01T12:33:59Z

...x.ui.scriptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ActionsViewTable.java

 			}
+		} catch (IllegalArgumentException e) {


Im not certain whether this should be a try catch or if it would be better as an exception, its my understanding that if this is non-exceptional flow control its preferred to use an if statement for this instead.

LowriJenkins · 2021-07-02T10:46:19Z

...iptgenerator/src/uk/ac/stfc/isis/ibex/ui/scriptgenerator/views/ScriptGeneratorViewModel.java

            }

            @Override
            public String getToolTipText(Object element) {
            return getScriptGenActionToolTipText(element);
            }
+
+            @Override
+        	public void update(ViewerCell cell) {


Looks like the indentation here is off, a few other places as well.

JamesKingWork · 2021-08-11T16:08:09Z

Adding a large number of rows quickly, and then changing the script definition to one with global variables causes the script genderator to crash.

I couldn't cause the script generator to crash doing this, but it did write nasty things to the log that looked like it could be the symptom of what would have caused a crash for you. I have now rewritten the algorithm to prevent this issue, I hope it also corrects the crashing that you saw, let me know if it doesn't so we can look through together to see what is causing it.

…umber doesn't update

LowriJenkins

With the changes made and handling of the edge case found, this is good to go in.

JamesKingWork added 7 commits June 22, 2021 14:24

Refactoring table to use the ActionProvider and control updates rathe…

1f5f420

…r than using setInput

Fix tab to new action bug and correct removal of actions

4f25286

Refresh list from a different place to refresh correctly

7f107d4

Remove no longer used changes

6b51cd4

Remove unused imports

72784e5

Rename method, remove unused logger

39e0a51

Refactor to move ActionProvider functionality into ActionsViewTable a…

7eb9618

…nd remove unused imports and remove author

JamesKingWork mentioned this pull request Jun 23, 2021

Script Generator: Make proper use of setInput [Timebox: 2 days] ISISComputingGroup/IBEX#5811

Closed

JamesKingWork commented Jun 23, 2021

View reviewed changes

JamesKingWork added 4 commits June 25, 2021 15:03

Correct how the table validity and estimated time are updated

75d95e3

Merge branch 'master' into Ticket5811_table_bug_fixing

8a4619e

Correct updating of estimated run time text

4493185

Remove actions all together so the viewer can cope

17fec2a

LowriJenkins self-assigned this Jun 30, 2021

LowriJenkins requested changes Jul 2, 2021

View reviewed changes

JamesKingWork added 2 commits July 20, 2021 15:53

Merge branch 'master' into Ticket5811_table_bug_fixing

99f2427

Use if instead of try catch

fe3ce2a

pull-request-size bot added the size/L label Jul 28, 2021

JamesKingWork added 4 commits August 5, 2021 13:22

Merge branch 'master' into Ticket5811_table_bug_fixing

bf4e171

Merge branch 'master' into Ticket5811_table_bug_fixing

44b1be8

Correct checkstyle and rename incorrect method

8f2b404

Change algorithm for removing actions from table to avoid crashes

83e7a06

Correct bug where we are unable to delete rows and where the action n…

1c0c20b

…umber doesn't update

JamesKingWork and others added 3 commits August 12, 2021 10:40

Remove accidental commenting of code

ce32518

Temp Fix for edge case when changing scripts quickly

1ee9644

Merge branch 'master' into Ticket5811_table_bug_fixing

fdcde39

LowriJenkins approved these changes Aug 20, 2021

View reviewed changes

LowriJenkins merged commit 80dba25 into master Aug 20, 2021

LowriJenkins deleted the Ticket5811_table_bug_fixing branch August 20, 2021 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket5811 table bug fixing #1318

Ticket5811 table bug fixing #1318

JamesKingWork commented Jun 23, 2021

JamesKingWork Jun 23, 2021 •

edited

Loading

JamesKingWork Jun 23, 2021

JamesKingWork Jun 23, 2021

JamesKingWork Jun 23, 2021

JamesKingWork Jun 23, 2021

JamesKingWork Jun 23, 2021

JamesKingWork Jun 23, 2021

LowriJenkins left a comment •

edited

Loading

LowriJenkins Jul 1, 2021

LowriJenkins Jul 2, 2021

JamesKingWork commented Aug 11, 2021

LowriJenkins left a comment

Ticket5811 table bug fixing #1318

Ticket5811 table bug fixing #1318

Conversation

JamesKingWork commented Jun 23, 2021

Description of work

Ticket

Acceptance criteria

To test:

Unit tests

System tests

Documentation

Code Review

Final Steps

JamesKingWork Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LowriJenkins left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesKingWork commented Aug 11, 2021

LowriJenkins left a comment

Choose a reason for hiding this comment

JamesKingWork Jun 23, 2021 •

edited

Loading

LowriJenkins left a comment •

edited

Loading