Fix PopupMenu item selction using keyboard, change event handling, #301

kotcrab · Jan 5, 2019 · a207f49 · a207f49
1 parent 1de9e2d
commit a207f49
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 13 deletions.
diff --git a/ui/CHANGES.md b/ui/CHANGES.md
@@ -1,4 +1,6 @@
 #### Version: 1.4.3-SNAPSHOT (LibGDX 1.9.9)
+- **Fixed**: Infinite loop in `PopupMenu` when trying to select next (or previous) `MenuItem` using keyboard and menu has no selectable `MenuItem`s.
+- **Changed**: `PopupMenu` keyboard events will be now treated as handled by scene2d (they won't be passed to application under the stage)
 
 #### Version: 1.4.2 (LibGDX 1.9.9)
 - Updated to libGDX 1.9.9

diff --git a/ui/src/main/java/com/kotcrab/vis/ui/widget/PopupMenu.java b/ui/src/main/java/com/kotcrab/vis/ui/widget/PopupMenu.java
@@ -109,32 +109,29 @@ public boolean touchDown (InputEvent event, float x, float y, int pointer, int b
 			@Override
 			public boolean keyDown (InputEvent event, int keycode) {
 				SnapshotArray<Actor> children = getChildren();
-
 				if (children.size == 0 || activeSubMenu != null) return false;
-
 				if (keycode == Input.Keys.DOWN) {
 					selectNextItem();
+					return true;
 				}
-
-				if (activeItem == null) return false;
-
 				if (keycode == Input.Keys.UP) {
 					selectPreviousItem();
+					return true;
 				}
-
+				if (activeItem == null) return false;
 				if (keycode == Input.Keys.LEFT && activeItem.containerMenu.parentSubMenu != null) {
 					activeItem.containerMenu.parentSubMenu.setActiveSubMenu(null);
+					return true;
 				}
-
 				if (keycode == Input.Keys.RIGHT && activeItem.getSubMenu() != null) {
 					activeItem.showSubMenu();
 					activeSubMenu.selectNextItem();
+					return true;
 				}
-
 				if (keycode == Input.Keys.ENTER) {
 					activeItem.fireChangeEvent();
+					return true;
 				}
-
 				return false;
 			}
 		};
@@ -191,7 +188,7 @@ private void removeHierarchy () {
 
 	private void selectNextItem () {
 		SnapshotArray<Actor> children = getChildren();
-		if (children.size == 0) return;
+		if (!hasSelectableMenuItems()) return;
 		int startIndex = activeItem == null ? 0 : children.indexOf(activeItem, true) + 1;
 		for (int i = startIndex; ; i++) {
 			if (i >= children.size) i = 0;
@@ -205,10 +202,10 @@ private void selectNextItem () {
 
 	private void selectPreviousItem () {
 		SnapshotArray<Actor> children = getChildren();
-		if (children.size == 0) return;
-		int startIndex = children.indexOf(activeItem, true) - 1;
+		if (!hasSelectableMenuItems()) return;
+		int startIndex = activeItem == null ? children.size - 1 : children.indexOf(activeItem, true) - 1;
 		for (int i = startIndex; ; i--) {
-			if (i == -1) i = children.size - 1;
+			if (i <= -1) i = children.size - 1;
 			Actor actor = children.get(i);
 			if (actor instanceof MenuItem && ((MenuItem) actor).isDisabled() == false) {
 				setActiveItem((MenuItem) actor, true);
@@ -217,6 +214,14 @@ private void selectPreviousItem () {
 		}
 	}
 
+	private boolean hasSelectableMenuItems () {
+		SnapshotArray<Actor> children = getChildren();
+		for (Actor actor : children) {
+			if (actor instanceof MenuItem && ((MenuItem) actor).isDisabled() == false) return true;
+		}
+		return false;
+	}
+
 	@Override
 	public <T extends Actor> Cell<T> add (T actor) {
 		if (actor instanceof MenuItem) {