PlotCommitList: Refactor lane creation and position allocation

This also properly removes the newly determinded lane position from the
freePositions set in handleBlockedLanes(). closeLane() does only recycle
active lanes, to avoid recycling lanes twice.

Change-Id: Icd019fcf7974441ed05686bb61d6de4e8bf4ab7c
Signed-off-by: Konrad Kügler <swamblumat-eclipsebugs@yahoo.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java b/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
index 9175e14f2d4285fa8fe9416984d5d0b1258bb1c9..9686e4568a946ab91ec150559ceacbae23d02d7b 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revplot/PlotCommitList.java
@@ -121,7 +121,6 @@ public class PlotCommitList<L extends PlotLane> extends
                final int nChildren = currCommit.getChildCount();
                if (nChildren == 0) {
                        currCommit.lane = nextFreeLane();
-                       activeLanes.add(currCommit.lane);
                        closeLane(currCommit.lane);
                        return;
                }
@@ -135,7 +134,6 @@ public class PlotCommitList<L extends PlotLane> extends
                                // Hmmph. This child must be the first along this lane.
                                //
                                c.lane = nextFreeLane();
-                               activeLanes.add(c.lane);
                        }
                        for (int r = index - 1; r >= 0; r--) {
                                final PlotCommit rObj = get(r);
@@ -175,7 +173,6 @@ public class PlotCommitList<L extends PlotLane> extends
                                // not already positioned.
                                if (c.lane == null) {
                                        c.lane = nextFreeLane();
-                                       activeLanes.add(c.lane);
                                        if (reservedLane != null)
                                                closeLane(c.lane);
                                        else
@@ -192,7 +189,6 @@ public class PlotCommitList<L extends PlotLane> extends
                                closeLane(reservedLane);
 
                        currCommit.lane = nextFreeLane();
-                       activeLanes.add(currCommit.lane);
 
                        handleBlockedLanes(index, currCommit, nChildren);
                }
@@ -218,32 +214,33 @@ public class PlotCommitList<L extends PlotLane> extends
                                if (--remaining == 0)
                                        break;
                        }
+                       addBlockedPosition(blockedPositions, rObj);
                        if (rObj != null) {
-                               PlotLane lane = rObj.getLane();
-                               if (lane != null)
-                                       blockedPositions.set(lane.getPosition());
                                rObj.addPassingLane(commit.lane);
                        }
                }
                // Now let's check whether we have to reposition the lane
                if (blockedPositions.get(commit.lane.getPosition())) {
-                       int newPos = -1;
-                       for (Integer pos : freePositions)
-                               if (!blockedPositions.get(pos.intValue())) {
-                                       newPos = pos.intValue();
-                                       break;
-                               }
-                       if (newPos == -1)
-                               newPos = positionsAllocated++;
+                       int newPos = getFreePosition(blockedPositions);
                        freePositions.add(Integer.valueOf(commit.lane.getPosition()));
                        commit.lane.position = newPos;
                        activeLanes.add(commit.lane);
                }
        }
 
+       private static void addBlockedPosition(BitSet blockedPositions,
+                       final PlotCommit rObj) {
+               if (rObj != null) {
+                       PlotLane lane = rObj.getLane();
+                       // Positions may be blocked by a commit on a lane.
+                       if (lane != null)
+                               blockedPositions.set(lane.getPosition());
+               }
+       }
+
        private void closeLane(PlotLane lane) {
-               recycleLane((L) lane);
                if (activeLanes.remove(lane)) {
+                       recycleLane((L) lane);
                        freePositions.add(Integer.valueOf(lane.getPosition()));
                }
        }
@@ -255,15 +252,37 @@ public class PlotCommitList<L extends PlotLane> extends
        }
 
        private PlotLane nextFreeLane() {
+               return nextFreeLane(null);
+       }
+
+       private PlotLane nextFreeLane(BitSet blockedPositions) {
                final PlotLane p = createLane();
-               if (freePositions.isEmpty()) {
-                       p.position = positionsAllocated++;
+               p.position = getFreePosition(blockedPositions);
+               activeLanes.add(p);
+               return p;
+       }
+
+       /**
+        * @param blockedPositions
+        *            may be null
+        * @return a free lane position
+        */
+       private int getFreePosition(BitSet blockedPositions) {
+               if (freePositions.isEmpty())
+                       return positionsAllocated++;
+
+               if (blockedPositions != null) {
+                       for (Integer pos : freePositions)
+                               if (!blockedPositions.get(pos.intValue())) {
+                                       freePositions.remove(pos);
+                                       return pos.intValue();
+                               }
+                       return positionsAllocated++;
                } else {
                        final Integer min = freePositions.first();
-                       p.position = min.intValue();
                        freePositions.remove(min);
+                       return min.intValue();
                }
-               return p;
        }
 
        /**