Cleanup in NGPopUpButton

Author: hugithordarson

ng-appserver/src/main/java/ng/appserver/templating/elements/NGPopUpButton.java

@@ -6,6 +6,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.SequencedCollection;
 
 import ng.appserver.NGContext;
 import ng.appserver.NGRequest;
@@ -18,34 +19,24 @@
 
 /**
  * FIXME: Add support for <optgroup> // Hugi 2024-07-13
+ * FIXME: Currently always using an item's index in the original 'list' as an option's "value". We're missing a 'value' association to allow a custom value to go through to the generated request parameters // Hugi 2023-05-01
+ * FIXME: Currently always replacing the value of the targeted "selections" binding with a new List. Having the option to modify an existing collection might be good.
  */
 
 public class NGPopUpButton extends NGDynamicElement {
 
 	private static final String NO_SELECTION_OPTION_VALUE = "NO_SELECTION_OPTION_VALUE";
 
 	private final NGAssociation _listAss;
-
 	private final NGAssociation _itemAss;
-
 	private final NGAssociation _displayStringAss;
-
 	private final NGAssociation _noSelectionStringAss;
-
-	private final NGAssociation _selectionAss;
-
 	private final NGAssociation _disabledAss;
-
 	private final NGAssociation _nameAss;
-
 	private final NGAssociation _indexAss;
-
-	/**
-	 * FIXME: We might want to offer the opportunity to replace the entire "selections" association in the target component, instead of modifying the existing collection // Hugi 2024-07-13
-	 */
-	private final NGAssociation _selectionsAss;
-
 	private final NGAssociation _multipleAss;
+	private final NGAssociation _selectionAss;
+	private final NGAssociation _selectionsAss;
 
 	/**
 	 * Pass-through attributes
@@ -73,23 +64,25 @@ public NGPopUpButton( String name, Map<String, NGAssociation> associations, NGEl
 	public void takeValuesFromRequest( NGRequest request, NGContext context ) {
 		if( !disabled( context ) ) {
 			final String name = name( context );
-			final List<String> valuesFromRequest = request.formValuesForKey( name );
+			final List<String> selectedValuesFromRequest = request.formValuesForKey( name );
 
-			// FIXME: We need to handle "empty" for multiple selects. We're currently not handling the "no item selected" case // Hugi 2025-05-25
-			if( !valuesFromRequest.isEmpty() ) {
-				if( multiple( context ) ) {
-					takeMultipleValuesFromRequest( context, name, valuesFromRequest );
-				}
-				else {
-					takeSingleValueFromRequest( context, name, valuesFromRequest );
-				}
+			if( multiple( context ) ) {
+				takeValuesFromRequestMultiple( context, name, selectedValuesFromRequest );
+			}
+			else {
+				takeValuesFromRequestSingle( context, name, selectedValuesFromRequest );
 			}
 		}
 	}
 
-	private void takeMultipleValuesFromRequest( final NGContext context, final String name, final List<String> valuesFromRequest ) {
+	private void takeValuesFromRequestMultiple( final NGContext context, final String name, final List<String> valuesFromRequest ) {
 
-		// FIXME: We might want to offer th user the opportunity that we modify the original collection instead of passing in a new one // Hugi 2024-07-13
+		// FIXME: OK, we have something of a problem with NGBrowser.
+		// Selecting no objects in the UI results in it not being represented in the request's formValues at all (no value present for it's 'name').
+		// This means that in the case of multiple forms on the same page and a form being submitted - we don't actually know if the value should be set to an empty list (we don't know if the containing form was submitted or if it was a different form).
+		// The only way I can think of to make this work is if we can check if the actual *form* containing the current browser is being submitted. In that case, we can definitely interpret the absence of a value as "no values selected".
+		// The current functionality means that if a browser is in one form on a page and a different form is submitted, the browser will always interpret that as "no values selected".
+		// Hugi 2025-08-04
 		final List<?> list = list( context );
 
 		final List selectedItems = new ArrayList<>();
@@ -101,24 +94,26 @@ private void takeMultipleValuesFromRequest( final NGContext context, final Strin
 		_selectionsAss.setValue( selectedItems, context.component() );
 	}
 
-	private void takeSingleValueFromRequest( final NGContext context, final String name, final List<String> valuesFromRequest ) {
+	private void takeValuesFromRequestSingle( final NGContext context, final String name, final List<String> valuesFromRequest ) {
 
-		// If multiple form values are present for the same field name, the potential for an error condition is probably high enough to just go ahead and fail.
-		if( valuesFromRequest.size() > 1 ) {
-			throw new IllegalStateException( "The request contains %s form values named '%s'. I can only handle one at a time. The values you sent me are (%s).".formatted( valuesFromRequest.size(), name, valuesFromRequest ) );
-		}
+		if( !valuesFromRequest.isEmpty() ) {
+			// If multiple form values are present for the same field name, the potential for an error condition is probably high enough to just go ahead and fail.
+			if( valuesFromRequest.size() > 1 ) {
+				throw new IllegalStateException( "The request contains %s form values named '%s'. I can only handle one at a time. The values you sent me are (%s).".formatted( valuesFromRequest.size(), name, valuesFromRequest ) );
+			}
 
-		final String stringValueFromRequest = valuesFromRequest.get( 0 );
+			final String stringValueFromRequest = valuesFromRequest.get( 0 );
 
-		// If nothing is selected, we push null to the selection
-		if( NO_SELECTION_OPTION_VALUE.equals( stringValueFromRequest ) ) {
-			_selectionAss.setValue( null, context.component() );
-		}
-		else {
-			final int selectionIndex = Integer.parseInt( stringValueFromRequest );
-			final List<?> list = list( context );
-			final Object selectedItem = list.get( selectionIndex );
-			_selectionAss.setValue( selectedItem, context.component() );
+			// If nothing is selected, we push null to the selection
+			if( NO_SELECTION_OPTION_VALUE.equals( stringValueFromRequest ) ) {
+				_selectionAss.setValue( null, context.component() );
+			}
+			else {
+				final int selectionIndex = Integer.parseInt( stringValueFromRequest );
+				final List<?> list = list( context );
+				final Object selectedItem = list.get( selectionIndex );
+				_selectionAss.setValue( selectedItem, context.component() );
+			}
 		}
 	}
 
@@ -174,28 +169,23 @@ public void appendToResponse( final NGResponse response, final NGContext context
 
 			boolean isSelected = false;
 
-			// FIXME: Hacky way to get the currently selected item. We should be reusing logic from takeValuesFromRequest() here
-			// FIXME: Handling of multiple selections should really only be in the elements that support those // Hugi 2025-05-25
 			if( multiple( context ) ) {
-				final List<String> selectedIndexes = context.request().formValuesForKey( name( context ) );
-				isSelected = selectedIndexes.contains( String.valueOf( index ) );
-
-				// FIXME: We're doing a separate round here to check for current selections in the associated list. This is spaghetti logic by now, fix up // Hugi 2025-05-25
-				final List selectedObjects = (List)_selectionsAss.valueInComponent( context.component() );
+				final List selections = (List)_selectionsAss.valueInComponent( context.component() );
 
-				if( selectedObjects != null && selectedObjects.contains( object ) ) {
+				if( selections != null && selections.contains( object ) ) {
 					isSelected = true;
 				}
 			}
 			else {
-				final String indexValue = context.request().formValueForKey( name( context ) );
+				final String selectedValue = context.request().formValueForKey( name( context ) );
 
-				if( indexValue != null && !indexValue.equals( NO_SELECTION_OPTION_VALUE ) && Integer.parseInt( indexValue ) == index ) {
+				if( selectedValue != null && !selectedValue.equals( NO_SELECTION_OPTION_VALUE ) && Integer.parseInt( selectedValue ) == index ) {
 					isSelected = true;
 				}
 
 				if( _selectionAss != null ) {
 					final Object selectedObject = _selectionAss.valueInComponent( context.component() );
+
 					if( object.equals( selectedObject ) ) {
 						isSelected = true;
 					}
@@ -204,7 +194,6 @@ public void appendToResponse( final NGResponse response, final NGContext context
 
 			final String selectedMarker = isSelected ? " selected" : "";
 
-			// FIXME: We're currently always using the index as "value". We're going to want to allow the passing in of a custom value attribute through the value binding // Hugi 2023-05-01
 			response.appendContentString( "<option value=\"%s\"%s>%s</option>".formatted( index, selectedMarker, displayString ) );
 			index++;
 		}
@@ -264,6 +253,11 @@ private List<?> list( final NGContext context ) {
 			return Arrays.asList( cast );
 		}
 
-		throw new IllegalArgumentException( "NGRepetition only accepts java.util.List and java Arrays. You sent me a %s".formatted( listAssociationValue.getClass() ) );
+		if( listAssociationValue instanceof SequencedCollection sc ) {
+			// CHECKME: Ideally we don't want to convert the sequenced collection to a new List. However, making it redundant requires us to rethink/redesign how we're currently iterate over the list (by index) so here we are // Hugi 2024-09-30
+			return List.of( sc.toArray() );
+		}
+
+		throw new IllegalArgumentException( "%s only accepts java.util.List, java arrays and java.util.SequencedCollection. You sent me a %s".formatted( getClass().getSimpleName(), listAssociationValue.getClass() ) );
 	}
 }