Add better checked support in MenuView

- Keeping checked states outside of items.
- Revamp generic structure
- Can toggle item checked states
- Relates #832

spring-shell-core/src/main/java/org/springframework/shell/component/view/control/MenuBarView.java

@@ -17,8 +17,10 @@ package org.springframework.shell.component.view.control;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashMap;
 import java.util.List;
 import java.util.ListIterator;
+import java.util.Map;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -50,6 +52,9 @@ public class MenuBarView extends BoxView {
 	private MenuView currentMenuView;
 	private int activeItemIndex = -1;
 
+	// Need to keep menuviews alive not to lose their states
+	private final Map<MenuBarItem, MenuView> menuViews = new HashMap<>();
+
 	/**
 	 * Construct menubar view with menubar items.
 	 *
@@ -202,14 +207,11 @@ public class MenuBarView extends BoxView {
 		}
 		else {
 			MenuBarItem item = items.get(activeItemIndex);
-			currentMenuView = buildMenuView(item);
+			currentMenuView = menuViews.computeIfAbsent(item, i -> buildMenuView(i));
 		}
 	}
 
 	private void closeCurrentMenuView() {
-		if (currentMenuView != null) {
-			currentMenuView.destroy();
-		}
 		currentMenuView = null;
 	}
 

spring-shell-core/src/main/java/org/springframework/shell/component/view/control/MenuView.java

@@ -18,7 +18,9 @@ package org.springframework.shell.component.view.control;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -47,6 +49,12 @@ public class MenuView extends BoxView {
 	private final List<MenuItem> items = new ArrayList<>();
 	private int activeItemIndex = -1;
 
+	// we support only one radio group
+	private MenuItem radioActive;
+
+	// keep checked states outside of items itself
+	private Set<MenuItem> checkedActive = new HashSet<>();
+
 	/**
 	 * Construct menu view with no initial menu items.
 	 */
@@ -137,11 +145,23 @@ public class MenuView extends BoxView {
 			}
 		}
 		for (MenuItem item : items) {
-			String prefix = hasCheck
-					? (item.getCheckStyle() != MenuItemCheckStyle.NOCHECK
-						? (item.isChecked() ? "[x] " : "[ ] ")
-						: "    ")
-					: "";
+			String prefix = hasCheck ? "    " : "";
+			if (item.checkStyle == MenuItemCheckStyle.RADIO) {
+				if (radioActive == item) {
+					prefix = "[x] ";
+				}
+				else {
+					prefix = "[ ] ";
+				}
+			}
+			else if (item.checkStyle == MenuItemCheckStyle.CHECKED) {
+				if (checkedActive.contains(item)) {
+					prefix = "[x] ";
+				}
+				else {
+					prefix = "[ ] ";
+				}
+			}
 			String text = prefix + item.getTitle();
 			if (activeItemIndex == i) {
 				writer2.text(text, rect.x(), y);
@@ -166,19 +186,70 @@ public class MenuView extends BoxView {
 		registerMouseBinding(MouseEvent.Type.Wheel | MouseEvent.Button.WheelUp, () -> move(-1));
 	}
 
+	/**
+	 * Request to handle current selected item to get opened.
+	 */
 	private void keySelect() {
-		MenuItem item = items.get(activeItemIndex);
-		dispatch(ShellMessageBuilder.ofView(this, MenuViewOpenSelectedItemEvent.of(this, item)));
-		if (item.getAction() != null) {
-			dispatchRunnable(item.getAction());
+		select();
+	}
+
+	/**
+	 * Request to handle mouse event.
+	 */
+	private void mouseSelect(MouseEvent event) {
+		log.trace("select({})", event);
+		setSelected(indexAtPosition(event.x(), event.y()));
+		select();
+	}
+
+	/**
+	 * Toggle {@link MenuItem} checked state.
+	 *
+	 * @param item the menu item to toggle
+	 */
+	public void toggle(MenuItem item) {
+		if (item.checkStyle == MenuItemCheckStyle.RADIO) {
+			radioActive = item;
+		}
+		else if (item.checkStyle == MenuItemCheckStyle.CHECKED) {
+			if (checkedActive.contains(item)) {
+				checkedActive.remove(item);
+			}
+			else {
+				checkedActive.add(item);
+			}
 		}
 	}
 
+	/**
+	 * From current selected item index, if applicable, dispatch
+	 * {@link MenuViewSelectedItemChangedEvent}.
+	 */
+	private void select() {
+		if (activeItemIndex > -1 && activeItemIndex < items.size()) {
+			MenuItem item = items.get(activeItemIndex);
+			if (item == null) {
+				return;
+			}
+			toggle(item);
+			dispatch(ShellMessageBuilder.ofView(this, MenuViewOpenSelectedItemEvent.of(this, item)));
+			if (item.getAction() != null) {
+				dispatchRunnable(item.getAction());
+			}
+		}
+	}
+
+	/**
+	 * Request to move selection up or down.
+	 */
 	private void move(int count) {
 		log.trace("move({})", count);
 		setSelected(activeItemIndex + count);
 	}
 
+	/**
+	 *
+	 */
 	private void setSelected(int index) {
 		if (index >= items.size()) {
 			activeItemIndex = 0;
@@ -195,14 +266,14 @@ public class MenuView extends BoxView {
 		}
 	}
 
-	private void mouseSelect(MouseEvent event) {
-		log.trace("select({})", event);
-		int x = event.x();
-		int y = event.y();
-		setSelected(indexAtPosition(x, y));
-		keySelect();
-	}
-
+	/**
+	 * Gets an index of a item at given position. Returns negative index if position
+	 * doesn't map to existing item.
+	 *
+	 * @param x the x coordinate
+	 * @param y the y coordinate
+	 * @return an index or negative if not found
+	 */
 	private int indexAtPosition(int x, int y) {
 		Rectangle rect = getRect();
 		if (!rect.contains(x, y)) {
@@ -249,7 +320,6 @@ public class MenuView extends BoxView {
 		private final String title;
 		private final MenuItemCheckStyle checkStyle;
 		private final List<MenuItem> items;
-		private boolean checked;
 		private Runnable action;
 
 		/**
@@ -353,24 +423,6 @@ public class MenuView extends BoxView {
 			return checkStyle;
 		}
 
-		/**
-		 * Sets a checked state.
-		 *
-		 * @param checked checked state
-		 */
-		public void setChecked(boolean checked) {
-			this.checked = checked;
-		}
-
-		/**
-		 * Gets a checked state.
-		 *
-		 * @return checked state
-		 */
-		public boolean isChecked() {
-			return checked;
-		}
-
 		/**
 		 * Gets sub menu items. This will be {@code null} if not constructed via
 		 * {@link Menu} as plain {@link MenuItem} can't have other items.

spring-shell-core/src/test/java/org/springframework/shell/component/view/control/MenuViewTests.java

@@ -221,26 +221,39 @@ class MenuViewTests extends AbstractViewTests {
 
 		@Test
 		void showsCheckedInRadio() {
-			sub5.setChecked(true);
 			view = new MenuView(new MenuItem[] { sub5, sub6 });
 			view.setShowBorder(true);
 			configure(view);
 			view.setRect(0, 0, 10, 10);
+
+			view.toggle(sub5);
 			view.draw(screen10x10);
 			assertThat(forScreen(screen10x10)).hasHorizontalText("[x] sub5", 0, 1, 9);
 			assertThat(forScreen(screen10x10)).hasHorizontalText("[ ] sub6", 0, 2, 9);
+
+			view.toggle(sub6);
+			view.draw(screen10x10);
+			assertThat(forScreen(screen10x10)).hasHorizontalText("[ ] sub5", 0, 1, 9);
+			assertThat(forScreen(screen10x10)).hasHorizontalText("[x] sub6", 0, 2, 9);
 		}
 
 		@Test
 		void showsCheckedInNonRadio() {
-			sub4.setChecked(true);
 			view = new MenuView(new MenuItem[] { sub3, sub4 });
 			view.setShowBorder(true);
 			configure(view);
 			view.setRect(0, 0, 10, 10);
+
+			view.toggle(sub4);
 			view.draw(screen10x10);
 			assertThat(forScreen(screen10x10)).hasHorizontalText("[ ] sub3", 0, 1, 9);
 			assertThat(forScreen(screen10x10)).hasHorizontalText("[x] sub4", 0, 2, 9);
+
+			view.toggle(sub3);
+			view.toggle(sub4);
+			view.draw(screen10x10);
+			assertThat(forScreen(screen10x10)).hasHorizontalText("[x] sub3", 0, 1, 9);
+			assertThat(forScreen(screen10x10)).hasHorizontalText("[ ] sub4", 0, 2, 9);
 		}
 
 		@Test