Progress on recursion safety

5 files changed, 52 insertions, 57 deletions

diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/AbstractTaskContainer.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/AbstractTaskContainer.java
index ec9ca6ff7..2f49438a6 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/AbstractTaskContainer.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/AbstractTaskContainer.java
@@ -8,6 +8,7 @@
 package org.eclipse.mylyn.tasks.core;
 
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArraySet;
 
@@ -23,11 +24,9 @@ import org.eclipse.mylyn.tasks.core.AbstractTask.PriorityLevel;
 public abstract class AbstractTaskContainer extends PlatformObject implements Comparable<AbstractTaskContainer> {
 
 	/**
-	 * @since 2.1
-	 * 
 	 * TODO: consider removing in 3.0 if handled by content provider
 	 */
-	public static final int MAX_SUBTASK_DEPTH = 10;
+	private static final int MAX_SUBTASK_DEPTH = 10;
 	
 	private String handleIdentifier = "";
 
@@ -64,8 +63,20 @@ public abstract class AbstractTaskContainer extends PlatformObject implements Co
 		children.clear();
 	}
 
+	/**
+	 * Removes any cyclic dependencies in children.
+	 * 
+	 * TODO: review to make sure that this is too expensive, or move to creation.
+	 */
 	public Set<AbstractTask> getChildren() {
-		return Collections.unmodifiableSet(children);
+		Set<AbstractTask> childrenWithoutCycles = new HashSet<AbstractTask>();
+		for (AbstractTask child : children) {
+			if (!child.contains(this.getHandleIdentifier())) {
+				childrenWithoutCycles.add(child);
+			}
+		}
+		return Collections.unmodifiableSet(childrenWithoutCycles);
+//		return Collections.unmodifiableSet(children);
 	}
 
 	/**
@@ -79,7 +90,6 @@ public abstract class AbstractTaskContainer extends PlatformObject implements Co
 
 	private boolean containsHelper(String handle, int depth) {
 		if (depth >= MAX_SUBTASK_DEPTH) {
-//			StatusHandler.fail(new Throwable("Max child depth reached for task: " + handle), "", false);
 			return false;
 		} else {
 			depth++;
diff --git a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/TaskList.java b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/TaskList.java
index edce7bef6..fbe26163d 100644
--- a/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/TaskList.java
+++ b/org.eclipse.mylyn.tasks.core/src/org/eclipse/mylyn/tasks/core/TaskList.java
@@ -695,8 +695,11 @@ public class TaskList {
 		return max;
 	}
 
+	/**
+	 * Maxes out at 20 recursions for safety.
+	 */
 	private int largestTaskIdHelper(Collection<AbstractTask> tasks, int lastMax, int depth) {
-		if (depth >= AbstractTaskContainer.MAX_SUBTASK_DEPTH) {
+		if (depth >= 20) {
 			return lastMax;
 		} else {
 			depth++;
diff --git a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/CustomTaskListDecorationDrawer.java b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/CustomTaskListDecorationDrawer.java
index 332f0993f..efc4e861e 100644
--- a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/CustomTaskListDecorationDrawer.java
+++ b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/CustomTaskListDecorationDrawer.java
@@ -29,7 +29,7 @@ import org.eclipse.swt.widgets.Listener;
 class CustomTaskListDecorationDrawer implements Listener {
 
 	private final TaskListView taskListView;
-	
+
 	private int activationImageOffset;
 
 	private Image taskActive = TasksUiImages.getImage(TasksUiImages.TASK_ACTIVE);
@@ -86,7 +86,7 @@ class CustomTaskListDecorationDrawer implements Listener {
 						drawSyncronizationImage((AbstractTaskContainer) data, event);
 					}
 				}
-				
+
 				// TODO: would be nice not to do this on each item's painting
 //				String text = tree.getFilterControl().getText();
 //				System.err.println(">>>>>> " + tree.getViewer().getExpandedElements().length);
@@ -94,7 +94,7 @@ class CustomTaskListDecorationDrawer implements Listener {
 //					int offsetY = tree.getViewer().getExpandedElements().length * tree.getViewer().getTree().getItemHeight();
 //					event.gc.drawText("Open search dialog...", 20, offsetY - 10);
 //				}
-				
+
 				break;
 			}
 			case SWT.PaintItem: {
@@ -104,7 +104,7 @@ class CustomTaskListDecorationDrawer implements Listener {
 				if (data instanceof AbstractTaskContainer) {
 					drawSyncronizationImage((AbstractTaskContainer) data, event);
 				}
-								
+
 				break;
 			}
 			}
@@ -150,26 +150,18 @@ class CustomTaskListDecorationDrawer implements Listener {
 	}
 
 	private boolean hasIncoming(AbstractTaskContainer container) {
-		return hasIncomingHelper(container, 0);
-	}
-
-	private boolean hasIncomingHelper(AbstractTaskContainer container, int depth) {
-		if (depth >= AbstractTaskContainer.MAX_SUBTASK_DEPTH) {
-			return false;
-		} else {
-			depth++;
-			for (AbstractTask task : container.getChildren()) {
-				if (task != null) {
-					AbstractTask containedRepositoryTask = task;
-					if (containedRepositoryTask.getSynchronizationState() == RepositoryTaskSyncState.INCOMING) {
-						return true;
-					} else if (task.getChildren() != null && task.getChildren().size() > 0 && hasIncomingHelper(task, depth)) {
-						return true;
-					}
+		for (AbstractTask task : container.getChildren()) {
+			if (task != null) {
+				AbstractTask containedRepositoryTask = task;
+				if (containedRepositoryTask.getSynchronizationState() == RepositoryTaskSyncState.INCOMING) {
+					return true;
+				} else if (task.getChildren() != null && task.getChildren().size() > 0
+						&& hasIncoming(task)) {
+					return true;
 				}
 			}
-			return false;
 		}
+		return false;
 	}
 
 	private void drawActivationImage(final int activationImageOffset, Event event, Image image) {
diff --git a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskElementLabelProvider.java b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskElementLabelProvider.java
index af45d66df..415ce55fd 100644
--- a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskElementLabelProvider.java
+++ b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskElementLabelProvider.java
@@ -339,25 +339,20 @@ public class TaskElementLabelProvider extends LabelProvider implements IColorPro
 			return false;
 		}
 
-		return showHasActiveChildHelper(container, 0);
+		return showHasActiveChildHelper(container);
 	}
 
-	private boolean showHasActiveChildHelper(AbstractTaskContainer container, int depth) {
-		if (depth >= AbstractTaskContainer.MAX_SUBTASK_DEPTH) {
-			return false;
-		} else {
-			depth++;
-			for (AbstractTaskContainer child : container.getChildren()) {
-				if (child instanceof AbstractTask && ((AbstractTask) child).isActive()) {
+	private boolean showHasActiveChildHelper(AbstractTaskContainer container) {
+		for (AbstractTaskContainer child : container.getChildren()) {
+			if (child instanceof AbstractTask && ((AbstractTask) child).isActive()) {
+				return true;
+			} else {
+				if (showHasActiveChildHelper(child)) {
 					return true;
-				} else {
-					if (showHasActiveChildHelper(child, depth)) {
-						return true;
-					}
 				}
 			}
-			return false;
 		}
+		return false;
 	}
 
 	private boolean showHasChildrenPastDue(AbstractTaskContainer container) {
@@ -365,25 +360,20 @@ public class TaskElementLabelProvider extends LabelProvider implements IColorPro
 			return false;
 		}
 
-		return showHasChildrenPastDueHelper(container, 0);
+		return showHasChildrenPastDueHelper(container);
 	}
 
-	private boolean showHasChildrenPastDueHelper(AbstractTaskContainer container, int depth) {
-		if (depth >= AbstractTaskContainer.MAX_SUBTASK_DEPTH) {
-			return false;
-		} else {
-			depth++;
-			for (AbstractTaskContainer child : container.getChildren()) {
-				if (child instanceof AbstractTask && ((AbstractTask) child).isPastReminder()
-						&& !((AbstractTask) child).isCompleted()) {
+	private boolean showHasChildrenPastDueHelper(AbstractTaskContainer container) {
+		for (AbstractTaskContainer child : container.getChildren()) {
+			if (child instanceof AbstractTask && ((AbstractTask) child).isPastReminder()
+					&& !((AbstractTask) child).isCompleted()) {
+				return true;
+			} else {
+				if (showHasChildrenPastDueHelper(child)) {
 					return true;
-				} else {
-					if (showHasChildrenPastDueHelper(child, depth)) {
-						return true;
-					}
 				}
 			}
-			return false;
 		}
+		return false;
 	}
 }
diff --git a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskListContentProvider.java b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskListContentProvider.java
index 3adb7bde7..52484a595 100644
--- a/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskListContentProvider.java
+++ b/org.eclipse.mylyn.tasks.ui/src/org/eclipse/mylyn/internal/tasks/ui/views/TaskListContentProvider.java
@@ -55,7 +55,7 @@ public class TaskListContentProvider extends AbstractTaskListContentProvider {
 	}
 
 	/**
-	 * returns first parent found
+	 * @return	 first parent found
 	 */
 	public Object getParent(Object child) {
 		// Return first parent found, first search within queries then categories.
@@ -88,8 +88,8 @@ public class TaskListContentProvider extends AbstractTaskListContentProvider {
 	 */
 	public boolean hasChildren(Object parent) {
 		if (parent instanceof AbstractRepositoryQuery) {
-			AbstractRepositoryQuery t = (AbstractRepositoryQuery) parent;
-			return !t.isEmpty();
+			AbstractRepositoryQuery query = (AbstractRepositoryQuery) parent;
+			return !query.isEmpty();
 		} else if (parent instanceof AbstractTask) {
 			return taskHasUnfilteredChildren((AbstractTask) parent);
 		} else if (parent instanceof AbstractTaskContainer) {