copyedit gui coding section

Author: Tom-Willemsen

doc/client/coding/Adding-a-Button-to-the-Perspective-Switcher.md

@@ -1,4 +1,4 @@
-# Adding a plugin or feature to Maven
+# Adding a plugin or feature
 
 The steps for adding a plug-in (one small part of IBEX, such as the blocks view) or feature (a larger collection of plug-ins, such as CSS) to the maven build are below:
 

doc/client/coding/Adding-a-Plugin-or-Feature-to-Maven-Build.md

@@ -1,10 +1,12 @@
 # Connecting a view to a PV
 
-See [An Introduction to Databinding](Databinding) for a explanation of databinding.
+:::{seealso}
+[An Introduction to Databinding](Databinding) for an explanation of databinding.
+:::
 
-The basic arrangement for the mechanism for connecting a View to a PV is shown below:
+The basic arrangement for the mechanism for connecting a View to a PV is:
 
-![Basic principle](basic_principle.png)
+<img src="connect_view_to_pv_basic_principle.png" width=750>
 
 ## Adding an Observable
 

doc/client/coding/Connecting-a-View-to-a-PV.md

@@ -1,10 +1,10 @@
 # Databinding
 
-Note: there are some helpful tips to common errors/mistakes [here](Databinding---common-mistakes).
+:::{seealso}
+[How to connect a view to a PV](Connecting-a-View-to-a-PV)
+:::
 
-Note: an explanation of how to connect a view to a PV is [here](Connecting-a-View-to-a-PV)
-
-## Create the example plug-ins ##
+## Create the example plug-ins
 
 First create an Eclipse plug-in for the back-end:
 
@@ -384,4 +384,47 @@ myViewer.setInput(BeanProperties.list("names").observe(myViewModel));
 List myList = myViewer.getList();
 ```
 
-`ListViewer` is a wrapper around the List control that provides extra functionality and `ObservableListContentProvider` make the databinding work.
+`ListViewer` is a wrapper around the List control that provides extra functionality and `ObservableListContentProvider` make the databinding work.
+
+## Troubleshooting
+
+### Missing getter or setter (or incorrectly named)
+
+The getter and setters (if the value can be set from the UI) must exist and be named correctly. For example: `getName` and `setName` for `BeanProperties.value("name")`.
+
+Behind the scenes the databinding library automatically changes the first character to upper-case and then prepends "get" and "set". 
+
+### Incorrectly cased string in binding
+
+With something like the following it is import that the name of the property to bind to is cased correctly:
+```java
+ctx.bindValue(WidgetProperties.text(SWT.Modify)
+            .observe(txtAge), BeanProperties.value("age").observe(person));
+```
+
+This assumes there is a getter and setter called `getAge` and `setAge`.
+
+For something like `getFedId` the binding code would look like:
+
+```java
+ctx.bindValue(WidgetProperties.text(SWT.Modify)
+            .observe(txtId), BeanProperties.value("fedId").observe(person));
+```
+
+:::{important}
+The 'f' of `fedId` is lower-case. It will not work if it is upper-case.
+:::
+
+### The getter or setter "silently" throws an exception
+
+If any code in the getter throws an unhandled exception then the binding won't work because the value cannot be read.
+If a setter throws an unhandled exception before the firing the property change then the listeners will not receive the change signal. Both result in the binding being broken.
+
+If a binding seems to work intermittently then there might be something in the getter or setter causing this, e.g. an object used in a getter that switches between being null and initialised based on something else.
+
+The exceptions will appear in the console inside Eclipse and IBEX, but won't cause an error pop-up to appear.
+
+### The binding works but the initial value is not put in the widget
+
+When the binding first happens it will call the getter to set the widget properties to whatever is in the model. 
+If this doesn't happen, the getter is probably non-existent or not implemented correctly. 

doc/client/coding/Databinding---common-mistakes.md

@@ -1,14 +1,15 @@
-# Conventions
+# GUI Coding Conventions
 
-Contains the style and coding conventions for the IBEX GUI.
+:::{note}
+New conventions should not be added to this document without first being discussed at a 
+[code chat](/processes/meetings/Code-Chats-and-Lightning-Talks) or a group code-review session.
+:::
 
-**New conventions should not be added to this document without first being discussed at a 'Code Chat'.**
+## Style Conventions
 
-## Style Conventions #
+Unless stated otherwise below, we should follow the standard Java conventions for style where possible.
 
-Unless stated otherwise below we should follow the standard Java conventions for style where possible.
-
-## Code documentation ##
+## Documentation
 
 Classes and public methods should be documented using the Javadoc syntax. For example:
 
@@ -46,7 +47,7 @@ public class CustomPizza extends Pizza {
     }
 }
 ```
-For Getters it is okay to skip the first sentence and do the following as it saves repetition:
+For getters, it is acceptable to skip the first sentence and do the following as it saves repetition:
 ```java
 /**
  * @return true if the block is enabled
@@ -56,9 +57,16 @@ public boolean getEnabled() {
 }
 ```
 
-## Code formatting ##
+Inline comments should have a space between the // and the text, and start with a capital letter:
+```java
+// This is a good comment
+    
+//this is a bad comment
+```
 
-For Java use the standard conventions built in to the IBEX developer's version of Eclipse. 
+## Code formatting
+
+For Java use the standard conventions built in to Eclipse. 
 
 An example of what it looks like:
 ```java
@@ -78,58 +86,18 @@ void foo2() {
 ```
 In Eclipse, a quick way to auto-format the code correctly is to use Ctrl+Shift+F.
 
-## Code comments ##
-
-Comments should have a space between the // and the text, and start with a capital letter:
-```java
-// This is a good comment
-    
-//this is a bad comment
-```
-
-## Checkstyle ##
-
-Code should be run through Checkstyle via Eclipse and corrected (within reason) before being committed.
-The Checkstyle plug-in is installed as part of the IBEX developer's version of Eclipse.
-
-The Checkstyle configuration file for Eclipse is more picky that the one used on Jenkins as it warns about 'magic numbers' and 'Java-docs'.
-
-By right-clicking on a file one can tell Checkstyle to check that file.
+## Naming conventions
 
-Warnings must be fixed where possible.
+### General
 
-Checkstyle has a suppress warning flag that tells it to ignore certain warning; warnings that are allowed to be suppressed are:
+We use the [standard Java naming conventions](https://www.oracle.com/java/technologies/javase/codeconventions-namingconventions.html):
+- Class names are CamelCase and usually nouns, e.g. `FileReader` not `ReadsFile`
+- Method names are Mixed Case (also known as Lower CamelCase), e.g. `sayHello`
+- Package names are lowercase, e.g. `uk.ac.stfc.isis.dae`
+- Variable names are Mixed Case, e.g. `calculateMeanSpeed`
+- Constants are uppercase spaced by underscores, e.g. `SECONDS_PER_FORTNIGHT`
 
-* Magic numbers - if it is related to a GUI layout then suppress them.
-
-* Name must match pattern - ignore GUI names that don't match the recommended pattern (e.g. `gd_switchToCombo`)
-    
-Suppression example:
-
-```java
-@SuppressWarnings({ "checkstyle:magicnumber", "checkstyle:localvariablename" })
-public void getSecondsInHours(int hours) {
-    int seconds_per_hour = 60 * 60;    // Magic numbers and a bad variable name
-    return hours * seconds_per_hour;
-}
-```
-## Naming conventions ##
-
-### General ###
-
-In general we use the standard Java naming conventions, e.g.:
-
-* Class names are CamelCase and usually nouns, e.g. `FileReader` not `ReadsFile`
-
-* Method names are Mixed Case (also known as Lower CamelCase), e.g. `sayHello`
-
-* Package names are lowercase, e.g. `uk.ac.stfc.isis.dae`
-
-* Variable names are Mixed Case, e.g. `calculateMeanSpeed`
-
-* Constants are uppercase spaced by underscores, e.g. `SECONDS_PER_FORTNIGHT`
-
-### Getters and Setters ###
+### Getters and Setters
 
 Getters and Setters should follow the JavaBeans convention, namely:
 
@@ -163,45 +131,34 @@ class Point {
 }
 ```    
 
-## Coding Conventions ##
+## Coding Conventions
 
-A mix of IBEX specific and general Java coding conventions.
-
-### GUI code must use a View Model ###
+### GUI code should use a model-view-viewmodel (MVVM) pattern
 
 This maintains a separation between pure GUI code and the back-end. It also makes it easier for us to test our code.
-See the previous GUI Chat slides for more information.
-
-Some legacy code does not have a View Model, this is on the list to fix.
+See the [previous GUI Chat slides](/processes/meetings/Code-Chats-and-Lightning-Talks) for more information.
 
-### Use data-binding for GUI items ###
+### Use data-binding for GUI items
 
-~~For connecting UI elements to data from the View Model use data-binding. 
-It seems that if a  mix of data-binding and more traditional SWT wiring up is used (e.g. AddPropertyChangeListener) then the data-binding will stop working*, so always using data-binding should avoid this problem.~~
+For connecting UI elements to data from the View Model, prefer to use [databinding](Databinding) where possible.
 
-~~*This does need more investigation to find out why it occurs.~~
+### Don't mess with finalizers
 
-This no longer seems to apply.
-
-### Don't mess with finalizers ###
-From the Google Java Style Guide:
-
-```
-It is extremely rare to override Object.finalize.
-
-Tip: Don't do it. If you absolutely must, first read and understand Effective Java Item 7, "Avoid Finalizers," very carefully, and then don't do it.
-```
-
-As of Java 9, `finalize` is officially deprecated in java. The IBEX checkstyle configuration is configured to disallow finalizers - this check should not be disabled or overridden.
+In Java versions >=9, `finalize` is deprecated and marked for removal from Java. The IBEX checkstyle configuration
+is configured to disallow finalizers - this check must not be disabled or overridden.
 
 The remaining options for supported clean-up mechanisms (in preference order) are:
-- Implement `autocloseable` and use the class in a try-with-resources statement to ensure the relevant resource is closed
-- Use a `closeable` and manually call `close` to ensure the relevant resource is closed
+- Implement `AutoCloseable` and use the class in a try-with-resources statement to ensure the relevant resource is closed
+- Use a `Closeable` and manually call `close` to ensure the relevant resource is closed
 - Refactor to avoid needing to close the resource at all
-- Use a *supported* automatic clean-up mechanism such as `PhantomReference` or `Cleaner` only as a last resort. While these are *better* than finalizers, they still suffer from high complexity and are tricky to get right. In particular it is easy to accidentally create reference loops meaning that objects will never be cleaned.
+- Use a *supported* automatic clean-up mechanism such as `PhantomReference` or `Cleaner` only as a last resort.
+While these are *better* than finalizers, they still suffer from high complexity and are tricky to get right. 
+In particular, it is easy to accidentally create reference loops meaning that objects will never be cleaned.
+
+### Return a empty collection or stream, not null
 
-### Return a empty collection or stream, not null ###
-For methods that return arrays/lists/maps/sets/streams etc. don't return null. It is cleaner to return an empty instance as the calling code does not need to check for null.
+For methods that return arrays/lists/maps/sets/streams etc. don't return null. It is cleaner to return an empty
+instance as the calling code does not need to check for null.
 
 ```java
 // Avoids this extra check in the caller
@@ -211,14 +168,18 @@ if (cars != null && cars.contains("mustang") {
 ```
 See Effective Java Item 43 "Return empty arrays or collections, not nulls"
 
-### Prefer StringBuilder for string concatenation ###
-Using `+` is fine for, say, joining two or three short strings but it is inefficient for larger numbers of strings and longer strings. Use StringBuilder instead.
+### Prefer StringBuilder for string concatenation
+
+Using `+` is fine for, say, joining two or three short strings, but it is inefficient for larger numbers of strings and
+longer strings. Use `StringBuilder` instead.
 
 See Effective Java Item 51 "Beware the performance of string concatenation"
 
 ### Prefer `Optional` over `null`
 
-New APIs should not return null to indicate a missing value. Instead, return an Optional wrapping the value which may not exist. Where external code returns null to indicate a missing value, this should be wrapped in an optional as soon as reasonable.
+New APIs should not return null to indicate a missing value. Instead, return an `Optional` wrapping the value which may
+not exist. Where external code returns null to indicate a missing value, this should be wrapped in an optional as soon
+as reasonable.
 
 To convert a maybe-null value to an optional, use:
 ```java
@@ -236,7 +197,7 @@ String str = mightNotExist.orElse(null);
 
 Streams should be used where they make an algorithm clearer.
 
-When using streams, put each operation on it's own line.
+When using streams, put each operation on its own line.
 
 Good:
 ```java
@@ -254,5 +215,3 @@ public Stream<String> getNames() {
     return getThings().map(thing -> thing.getName()).filter(name -> name != "").sorted();
 }
 ```
-
-