OF-35 Improvements to form structure and bug fixes

* Moved detail text to under the field label i.e. left of the field.
* the add user/group text box has been moved to the bottom row of the
  repective table, and table headings have been changed.
* Fixed bug where blank properties were causing empty jids to be populated
  for sysadmins and allowedToCreate on the PubSubModule.
* Fixed bug where options were removed from the forms after users/groups
  were added (options were not being transfered onto the new form).

src/i18n/openfire_i18n_en.properties

@@ -856,6 +856,7 @@ global.test=Test
 global.click_test=Click to test...
 global.csrf.failed=CSRF Error: No changes made, you'll need to retry.
 global.update=Update
+global.remove=Remove
 
 # Group Chat Service Properties Page
 
@@ -2778,9 +2779,7 @@ pubsub.form.user=User
 
 pubsub.form.not_valid="{0}" is not a valid {1}.
 pubsub.form.already_in_list={0} "{1}" already in list.
-pubsub.form.no_data=No values set
-
-pubsub.form.add=Add {0}:
+pubsub.form.action=Action
 
 
 # Plugins Admin Page

src/java/org/jivesoftware/openfire/pubsub/PubSubModule.java

@@ -406,7 +406,7 @@ public class PubSubModule extends BasicModule implements ServerItemsProvider, Di
         // Load the list of JIDs that are sysadmins of the PubSub service
         String property = JiveGlobals.getProperty("xmpp.pubsub.sysadmin.jid");
         String[] jids;
-        if (property != null) {
+        if (property != null && !property.isEmpty()) {
             jids = property.split(",");
             for (String jid : jids) {
                 sysadmins.add(jid.trim().toLowerCase());
@@ -415,7 +415,7 @@ public class PubSubModule extends BasicModule implements ServerItemsProvider, Di
         nodeCreationRestricted = JiveGlobals.getBooleanProperty("xmpp.pubsub.create.anyone", false);
         // Load the list of JIDs that are allowed to create nodes
         property = JiveGlobals.getProperty("xmpp.pubsub.create.jid");
-        if (property != null) {
+        if (property != null && !property.isEmpty()) {
             jids = property.split(",");
             for (String jid : jids) {
                 allowedToCreate.add(jid.trim().toLowerCase());

src/java/org/jivesoftware/openfire/pubsub/PubSubServiceInfo.java

@@ -149,40 +149,37 @@ public class PubSubServiceInfo {
         for (FormField field : form.getFields()) {
 
             if (excludedFields == null || !excludedFields.contains(field.getVariable())) {
+
+                FormField completedField = completedForm.addField(field.getVariable(), field.getLabel(), field.getType());
+
                 switch (field.getType()) {
                 case boolean_type:
-                    FormField boolField = completedForm.addField(field.getVariable(), field.getLabel(),
-                            field.getType());
-                    boolField.addValue(ParamUtils.getBooleanParameter(request, field.getVariable()));
+                    completedField.addValue(ParamUtils.getBooleanParameter(request, field.getVariable()));
                     break;
                 case jid_multi:
-                    FormField multiJidField = completedForm.addField(field.getVariable(), field.getLabel(),
-                            field.getType());
                     for (String param : ParamUtils.getParameters(request, field.getVariable())) {
-                        multiJidField.addValue(param);
+                        completedField.addValue(param);
                     }
                     break;
                 case list_multi:
-                    FormField multiListField = completedForm.addField(field.getVariable(), field.getLabel(),
-                            field.getType());
                     for (String param : ParamUtils.getParameters(request, field.getVariable())) {
-                        multiListField.addValue(param);
+                        completedField.addValue(param);
                     }
                     break;
                 case list_single:
-                    FormField listField = completedForm.addField(field.getVariable(), field.getLabel(),
-                            field.getType());
-                    listField.addValue(ParamUtils.getParameter(request, field.getVariable()));
+                    completedField.addValue(ParamUtils.getParameter(request, field.getVariable()));
                     break;
                 case text_single:
-                    FormField textField = completedForm.addField(field.getVariable(), field.getLabel(),
-                            field.getType());
-                    textField.addValue(ParamUtils.getParameter(request, field.getVariable()));
+                    completedField.addValue(ParamUtils.getParameter(request, field.getVariable()));
                     break;
                 default:
-                    completedForm.addField(field.getVariable(), field.getLabel(), field.getType());
                     break;
                 }
+
+                for(FormField.Option option: field.getOptions()) {
+                    completedField.addOption(option.getLabel(), option.getValue());
+                }
+
             }
         }
         return completedForm;

src/web/pubsub-form-table.jsp

@@ -53,29 +53,29 @@ detailPreFix - property prefix for additional detail to be displayed against the
 		<br>
 	</c:forEach>
 </c:if>
-<table cellpadding="3" cellspacing="0" border="0" width="100%">
+<table cellpadding="3" cellspacing="0" border="0" width="1%">
 	<tbody>
 		<c:forEach var="field" items="${requestScope.fields}">
 			<c:if
 				test="${not requestScope.nonDisplayFields.contains(field.variable)}">
 				<tr>
-					<td nowrap width="1%"><label for="${field.variable}">${field.label}</label></td>
+					<td nowrap style="min-width: 300px"><label
+						style="font-weight: bold" for="${field.variable}">${field.label}</label></td>
 					<c:set var="isList"
 						value="${field.type.name() eq 'list_multi' or field.type.name() eq 'jid_multi'}" />
 					<c:choose>
 						<c:when test="${field.type.name() eq 'boolean_type'}">
-							<td width="1%" colspan="2"><input type="checkbox"
+							<td width="1%" rowspan="2"><input type="checkbox"
 								name="${field.variable}" id="${field.variable}"
-								${field.firstValue == 1 ? 'checked="checked"' : '' } />
-							</td>
+								${field.firstValue == 1 ? 'checked="checked"' : '' } /></td>
 						</c:when>
 						<c:when test="${field.type.name() eq 'text_single'}">
-							<td width="1%" colspan="2"><input type="text"
+							<td width="1%" rowspan="2"><input type="text"
 								name="${field.variable}" id="${field.variable}"
 								value="${field.firstValue}" style="width: 200px;" /></td>
 						</c:when>
 						<c:when test="${field.type.name() eq 'list_single'}">
-							<td width="1%" colspan="2"><select name="${field.variable}"
+							<td width="1%" rowspan="2"><select name="${field.variable}"
 								id="${field.variable}" style="width: 200px;">
 									<c:forEach var="option" items="${field.options}">
 										<option value="${option.value}"
@@ -86,7 +86,7 @@ detailPreFix - property prefix for additional detail to be displayed against the
 							</select></td>
 						</c:when>
 						<c:when test="${isList and not empty field.options}">
-							<td width="1%" colspan="2"><select name="${field.variable}"
+							<td width="1%" rowspan="2"><select name="${field.variable}"
 								id="${field.variable}" style="width: 200px;" multiple>
 									<c:forEach var="option" items="${field.options}">
 										<option value="${option.value}"
@@ -101,23 +101,18 @@ detailPreFix - property prefix for additional detail to be displayed against the
 								</button></td>
 						</c:when>
 						<c:when test="${isList and empty field.options}">
-							<td>
+							<td rowspan="2">
 								<div class="jive-table">
 									<table id="${field.variable}" cellpadding="0" cellspacing="0"
 										border="0" width="100%">
 										<thead>
 											<tr>
-												<th scope="col">value</th>
-												<th scope="col">remove</th>
+												<th scope="col"><fmt:message
+														key="pubsub.form.${listTypes[field.variable]}" /></th>
+												<th scope="col"><fmt:message key="pubsub.form.action" /></th>
 											</tr>
 										</thead>
 										<tbody>
-											<c:if test="${empty field.values}">
-												<tr>
-													<td align="center" colspan="2"><fmt:message
-															key="pubsub.form.no_data" /></td>
-												</tr>
-											</c:if>
 											<c:forEach var="value" items="${field.values}"
 												varStatus="loop">
 												<tr id="${field.variable}${loop.index}">
@@ -129,23 +124,23 @@ detailPreFix - property prefix for additional detail to be displayed against the
 													</td>
 												</tr>
 											</c:forEach>
-										</tbody>
-									</table>
-								</div>
-							</td>
-							<td><fmt:message var="paramVal"
-									key="pubsub.form.${listTypes[field.variable]}" /> <fmt:message
-									key="pubsub.form.add">
-									<fmt:param value="${paramVal}" />
-								</fmt:message> <br> <input type="text" size="45"
+											<tr>
+												<td><input type="text" style="width: 200px;"
 													id="${field.variable}-Additional"
 													name="${field.variable}-Additional"
-								onkeypress="detect_enter_keyboard(event)" /> <br> <input
-								type="submit" id="${field.variable}-Add"
+													onkeypress="detect_enter_keyboard(event)" /></td>
+												<td><input type="submit" id="${field.variable}-Add"
 													name="${field.variable}-Add"
 													value="<fmt:message key="global.add" />"></td>
+											</tr>
+										</tbody>
+									</table>
+								</div>
+							</td>
 						</c:when>
 					</c:choose>
+				</tr>
+				<tr>
 					<td><fmt:message var="detail"
 							key="${param.detailPreFix}.${fn:substringAfter(field.variable, '#')}" />
 						<c:if test="${not fn:startsWith(detail, '???')}">