Rework properties handling in rule table in preferences

Fixes #191

Author: adangel

ReleaseNotes.md

@@ -16,6 +16,8 @@ This is a minor release.
 ### Fixed Issues
 * [#192](https://github.com/pmd/pmd-eclipse-plugin/pull/192): Add japicmp
 
+* [#191](https://github.com/pmd/pmd-eclipse-plugin/issues/191): Rule Configuration Pref Page - PropertyDescriptor cannot be cast to Comparable
+
 ### API Changes
 
 ### External Contributions

net.sourceforge.pmd.eclipse.plugin/src/main/java/net/sourceforge/pmd/eclipse/ui/dialogs/NewPropertyDialog.java

@@ -4,12 +4,10 @@
 
 package net.sourceforge.pmd.eclipse.ui.dialogs;
 
-import java.lang.reflect.Method;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Map.Entry;
 
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.jface.dialogs.IDialogConstants;
@@ -34,6 +32,7 @@
 import net.sourceforge.pmd.eclipse.ui.preferences.br.SizeChangeListener;
 import net.sourceforge.pmd.eclipse.ui.preferences.br.ValueChangeListener;
 import net.sourceforge.pmd.eclipse.ui.preferences.editors.SWTUtil;
+import net.sourceforge.pmd.eclipse.ui.preferences.internal.PropertyEditorFactory;
 import net.sourceforge.pmd.eclipse.util.Util;
 import net.sourceforge.pmd.lang.rule.Rule;
 import net.sourceforge.pmd.lang.rule.RuleReference;
@@ -42,16 +41,14 @@
 import net.sourceforge.pmd.properties.PropertySource;
 
 /**
- * Implements a dialog for adding or editing a rule property. As the user
- * changes the specified type, each type's associated editor factory will
- * provide additional labels & widgets intended to capture other metadata.
+ * Implements a dialog for adding or editing a rule property.
  *
  * @author Brian Remedios
  */
 public class NewPropertyDialog extends TitleAreaDialog implements SizeChangeListener {
 
     private Text nameField;
-    private Text labelField;
+    private Text descriptionField;
     private Combo typeField;
     private Control[] factoryControls;
     private Composite dlgArea;
@@ -60,43 +57,56 @@ public class NewPropertyDialog extends TitleAreaDialog implements SizeChangeList
 
     private PropertySource propertySource;
     private PropertyDescriptor<?> descriptor;
-    private Map<Class<?>, EditorFactory<?>> editorFactoriesByValueType;
 
     // these are the ones we've tested, the others may work but might not make
     // sense in the xpath source context...
-    private static final Class<?>[] VALID_EDITOR_TYPES = new Class[] { String.class, Integer.class, Boolean.class,
-        Class.class, Method.class };
+    private static final Class<?>[] VALID_EDITOR_TYPES = new Class[] { String.class, Integer.class, Boolean.class };
     private static final Class<?> DEFAULT_EDITOR_TYPE = VALID_EDITOR_TYPES[0]; // first one
 
     /**
      * Constructor for RuleDialog. Supply a working descriptor with name &
      * description values we expect the user to change.
-     *
-     * @param parentdlgArea
      */
-    public NewPropertyDialog(Shell parent, Map<Class<?>, EditorFactory<?>> theEditorFactoriesByValueType,
-            PropertySource theSource, ValueChangeListener theChangeListener) {
+    public NewPropertyDialog(Shell parent, PropertySource theSource, ValueChangeListener theChangeListener) {
         super(parent);
 
         setShellStyle(SWT.DIALOG_TRIM | SWT.RESIZE | SWT.MAX | SWT.APPLICATION_MODAL);
 
         propertySource = theSource;
         changeListener = theChangeListener;
-        editorFactoriesByValueType = withOnly(theEditorFactoriesByValueType, VALID_EDITOR_TYPES);
+    }
+
+    /**
+     * Constructor for RuleDialog. Supply a working descriptor with name &
+     * description values we expect the user to change.
+     *
+     * @param parentdlgArea
+     * @deprecated use {@link #NewPropertyDialog(Shell, PropertySource, ValueChangeListener)} instead.
+     */
+    @Deprecated // for removal
+    public NewPropertyDialog(Shell parent, Map<Class<?>, EditorFactory<?>> theEditorFactoriesByValueType,
+            PropertySource theSource, ValueChangeListener theChangeListener) {
+        this(parent, theSource, theChangeListener);
     }
 
     /**
      * Constructor for RuleDialog.
      *
      * @param parentdlgArea
+     * @deprecated This ctor is not used and will be removed
      */
+    @Deprecated // for removal
     public NewPropertyDialog(Shell parent, Map<Class<?>, EditorFactory<?>> theEditorFactoriesByValueType, Rule theRule,
             PropertyDescriptor<?> theDescriptor, ValueChangeListener theChangeListener) {
         this(parent, theEditorFactoriesByValueType, theRule, theChangeListener);
 
         descriptor = theDescriptor;
     }
 
+    /**
+     * @deprecated This was a helper method that should have been private
+     */
+    @Deprecated // for removal
     public static Map<Class<?>, EditorFactory<?>> withOnly(Map<Class<?>, EditorFactory<?>> factoriesByType,
             Class<?>[] legalTypeKeys) {
         Map<Class<?>, EditorFactory<?>> results = new HashMap<>(legalTypeKeys.length);
@@ -125,8 +135,8 @@ protected Control createDialogArea(Composite parent) {
         nameField = buildNameText(dlgArea); // TODO i18l label
         buildLabel(dlgArea, "Datatype:");
         typeField = buildTypeField(dlgArea); // TODO i18l label
-        buildLabel(dlgArea, "Label:");
-        labelField = buildLabelField(dlgArea); // TODO i18l label
+        buildLabel(dlgArea, "Description:");
+        descriptionField = buildDescriptionField(dlgArea); // TODO i18l label
 
         setPreferredName();
         setInitialType();
@@ -182,24 +192,21 @@ public void handleEvent(Event event) {
         return text;
     }
 
-    private boolean isValidNewLabel(String labelCandidate) {
-        return StringUtils.isNotBlank(labelCandidate);
+    private boolean isValidNewDescription(String descriptionCandidate) {
+        return StringUtils.isNotBlank(descriptionCandidate);
     }
 
-    private boolean isPreExistingLabel(String labelCandidate) {
+    private boolean isPreExistingDescription(String descriptionCandidate) {
 
         for (PropertyDescriptor<?> desc : propertySource.getPropertyDescriptors()) {
-            if (desc.description().equalsIgnoreCase(labelCandidate)) {
+            if (desc.description().equalsIgnoreCase(descriptionCandidate)) {
                 return false;
             }
         }
         return true;
     }
 
-    /**
-     * Build the rule name text.
-     */
-    private Text buildLabelField(Composite parent) {
+    private Text buildDescriptionField(Composite parent) {
 
         final Text text = new Text(parent, SWT.SINGLE | SWT.BORDER);
         setFieldLayoutData(text);
@@ -220,24 +227,6 @@ private static String labelFor(Class<?> type) {
         return Util.signatureFor(type, new String[0]);
     }
 
-    /**
-     * A bit of a hack but this avoids the need to create an alternate lookup
-     * structure.
-     *
-     * @param label
-     * @return
-     */
-    private EditorFactory<?> factoryFor(String label) {
-
-        for (Entry<Class<?>, EditorFactory<?>> entry : editorFactoriesByValueType.entrySet()) {
-            if (label.equals(labelFor(entry.getKey()))) {
-                return entry.getValue();
-            }
-        }
-
-        return null;
-    }
-
     /**
      * Build the rule name text.
      */
@@ -246,20 +235,17 @@ private Combo buildTypeField(final Composite parent) {
         final Combo combo = new Combo(parent, SWT.READ_ONLY);
         setFieldLayoutData(combo);
 
-        String[] labels = new String[editorFactoriesByValueType.size()];
-        int i = 0;
-        for (Entry<Class<?>, EditorFactory<?>> entry : editorFactoriesByValueType.entrySet()) {
-            labels[i++] = labelFor(entry.getKey());
+        String[] labels = new String[VALID_EDITOR_TYPES.length];
+        for (int i = 0; i < labels.length; i++) {
+            labels[i] = VALID_EDITOR_TYPES[i].getSimpleName();
         }
         Arrays.sort(labels);
         combo.setItems(labels);
 
         combo.addSelectionListener(new SelectionAdapter() {
             @Override
             public void widgetSelected(SelectionEvent e) {
-                int selectionIdx = combo.getSelectionIndex();
-                EditorFactory<?> factory = factoryFor(combo.getItem(selectionIdx));
-
+                EditorFactory<?> factory = PropertyEditorFactory.INSTANCE;
                 factory(factory);
             }
         });
@@ -309,10 +295,9 @@ private void setPreferredName() {
     }
 
     private void setInitialType() {
-
         String editorLabel = labelFor(DEFAULT_EDITOR_TYPE);
         typeField.select(Util.indexOf(typeField.getItems(), editorLabel));
-        factory(factoryFor(editorLabel));
+        factory(PropertyEditorFactory.INSTANCE);
     }
 
     private void cleanFactoryStuff() {
@@ -323,13 +308,11 @@ private void cleanFactoryStuff() {
         }
     }
 
-    private <T> void factory(EditorFactory<T> theFactory) {
-
+    private void factory(EditorFactory<?> theFactory) {
         factory = theFactory;
-        // dummy values (??) that will be replaced
-        PropertyDescriptor<T> theDescriptor = theFactory.createDescriptor("??", "??", null);
+        PropertyDescriptor theDescriptor = theFactory.createDescriptor("propertyName", "The description", null);
         descriptor = theDescriptor;
-        labelField.setText(theDescriptor.description());
+        descriptionField.setText(theDescriptor.description());
         cleanFactoryStuff();
 
         factoryControls = theFactory.createOtherControlsOn(dlgArea, theDescriptor, propertySource, changeListener, this);
@@ -339,16 +322,6 @@ private <T> void factory(EditorFactory<T> theFactory) {
         dlgArea.getParent().pack();
     }
 
-    // /**
-    // * Helper method to shorten message access
-    // *
-    // * @param key a message key
-    // * @return requested message
-    // */
-    // private String getMessage(String key) {
-    // return PMDPlugin.getDefault().getStringTable().getString(key);
-    // }
-
     /**
      * @see org.eclipse.jface.dialogs.Dialog#okPressed()
      */
@@ -364,7 +337,7 @@ protected void okPressed() {
      * Perform the form validation
      */
     private boolean validateForm() {
-        boolean isOk = validateName() && validateLabel();
+        boolean isOk = validateName() && validateDescription();
         Control button = getButton(IDialogConstants.OK_ID);
         if (button != null) {
             button.setEnabled(isOk);
@@ -394,24 +367,21 @@ private boolean validateName() {
         return true;
     }
 
-    /**
-     * Perform the label validation
-     */
-    private boolean validateLabel() {
+    private boolean validateDescription() {
 
-        String label = labelField.getText().trim();
+        String label = descriptionField.getText().trim();
 
         if (StringUtils.isBlank(label)) {
             setErrorMessage("A descriptive label is required");
             return false;
         }
 
-        if (!isValidNewLabel(label)) {
+        if (!isValidNewDescription(label)) {
             setErrorMessage("Invalid label");
             return false;
         }
 
-        if (!isPreExistingLabel(label)) {
+        if (!isPreExistingDescription(label)) {
             setErrorMessage("Label text must differ from other label text");
             return false;
         }
@@ -431,8 +401,7 @@ public PropertyDescriptor<?> descriptor() {
     }
 
     private PropertyDescriptor<?> newDescriptor() {
-
-        return factory.createDescriptor(nameField.getText().trim(), labelField.getText().trim(), factoryControls);
+        return factory.createDescriptor(nameField.getText().trim(), descriptionField.getText().trim(), factoryControls);
     }
 
     @Override

net.sourceforge.pmd.eclipse.plugin/src/main/java/net/sourceforge/pmd/eclipse/ui/preferences/br/ImageColumnDescriptor.java

@@ -25,7 +25,13 @@ public class ImageColumnDescriptor extends AbstractRuleColumnDescriptor {
 
     public static final RuleFieldAccessor PROPERTIES_ACCESSOR = new BasicRuleFieldAccessor() {
         @Override
-        public Comparable<?> valueFor(Rule rule) {
+        public String labelFor(Rule rule) {
+            IndexedString value = valueFor(rule);
+            return value == null ? "" : value.string;
+        }
+
+        @Override
+        public IndexedString valueFor(Rule rule) {
             // notes indices of non-default values in the string for emphasis
             // during later rendering
             return RuleUIUtil.indexedPropertyStringFrom(rule); 

net.sourceforge.pmd.eclipse.plugin/src/main/java/net/sourceforge/pmd/eclipse/ui/preferences/br/PMDPreferencePage2.java

@@ -64,13 +64,18 @@ public class PMDPreferencePage2 extends AbstractPMDPreferencePage
     private Composite contentPanel;
 
     // columns shown in the rule treetable in the desired order
-    public static final RuleColumnDescriptor[] AVAILABLE_COLUMNS = new RuleColumnDescriptor[] { RuleTableColumns.NAME,
+    public static final RuleColumnDescriptor[] AVAILABLE_COLUMNS = new RuleColumnDescriptor[] {
+        RuleTableColumns.NAME,
         // PreferenceTableColumns.priorityName,
         // IconColumnDescriptor.priority,
         RuleTableColumns.IMG_PRIORITY,
         // PreferenceTableColumns.fixCount,
-        RuleTableColumns.SINCE, RuleTableColumns.RULE_SET_NAME, RuleTableColumns.RULE_TYPE, RuleTableColumns.MIN_LANGUAGE_VERSION,
-        RuleTableColumns.MAX_LANGUAGE_VERSION, RuleTableColumns.LANGUAGE,
+        RuleTableColumns.SINCE,
+        RuleTableColumns.RULE_SET_NAME,
+        RuleTableColumns.RULE_TYPE,
+        RuleTableColumns.MIN_LANGUAGE_VERSION,
+        RuleTableColumns.MAX_LANGUAGE_VERSION,
+        RuleTableColumns.LANGUAGE,
         // regex text -> compact color squares (for comparison)
         RuleTableColumns.FILTER_VIOLATION_REGEX,
         // xpath text -> compact color circles (for comparison)