Split queue comparators into a class and test.

This allows the logic to be expressed more nicely and tested
independently of the handlers. It also paves the way for a
bit of refactoring to reduce the number of random things
that need to be comparable...

irc/src/com/dmdirc/parser/irc/outputqueue/PriorityQueueHandler.java

@@ -23,6 +23,7 @@
 package com.dmdirc.parser.irc.outputqueue;
 
 import java.io.PrintWriter;
+import java.time.Duration;
 import java.util.concurrent.BlockingQueue;
 
 /**
@@ -37,8 +38,14 @@ public class PriorityQueueHandler extends QueueHandler {
      * @param queue Queue to use
      * @param out Output Stream to use
      */
-    public PriorityQueueHandler(final OutputQueue outputQueue, final BlockingQueue<QueueItem> queue, final PrintWriter out) {
-        super(outputQueue, queue, out);
+    public PriorityQueueHandler(
+            final OutputQueue outputQueue,
+            final BlockingQueue<QueueItem> queue,
+            final PrintWriter out) {
+        super(outputQueue,
+                queue,
+                QueueComparators.byPriorityThenNumber(Duration.ofSeconds(10)),
+                out);
     }
 
     /**
@@ -62,4 +69,5 @@ public class PriorityQueueHandler extends QueueHandler {
             // Do nothing
         }
     }
+
 }

irc/src/com/dmdirc/parser/irc/outputqueue/QueueComparators.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2006-2016 DMDirc Developers
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package com.dmdirc.parser.irc.outputqueue;
+
+import com.dmdirc.parser.common.QueuePriority;
+
+import java.time.Clock;
+import java.time.LocalDateTime;
+import java.time.temporal.TemporalAmount;
+import java.util.Comparator;
+
+/**
+ * Provides comparators for comparing {@link QueueItem}s.
+ */
+public final class QueueComparators {
+
+    private QueueComparators() {
+        // Shouldn't be used
+    }
+
+    /**
+     * Provides a comparator that checks priority and item number.
+     *
+     * @return a comparator that compares items by their priority, and then their item number.
+     */
+    public static Comparator<QueueItem> byPriorityThenNumber() {
+        return Comparator.comparing(QueueItem::getPriority)
+                .thenComparing(QueueItem::getItemNumber);
+    }
+
+    /**
+     * Provides a comparator that checks priority and item number, with special handling for
+     * starved items.
+     *
+     * <p>If an item has been queued for longer than {@code starvationThreshold}, its priority
+     * will be effectively ignored (i.e., it will just be compared on item number). This ensures
+     * that lower priority items are not kept in the queue indefinitely if there is a constant
+     * stream of higher priority items.
+     *
+     * @param starvationThreshold The time an item must be queued for to be considered starved.
+     * @return a comparator that compares items by their priority unless they are starved, and then
+     * their item number.
+     */
+    public static Comparator<QueueItem> byPriorityThenNumber(
+            final TemporalAmount starvationThreshold) {
+        return byPriorityThenNumber(Clock.systemDefaultZone(), starvationThreshold);
+    }
+
+    /**
+     * Provides a comparator that checks priority and item number, with special handling for
+     * starved items.
+     *
+     * <p>If an item has been queued for longer than {@code starvationThreshold}, its priority
+     * will be effectively ignored (i.e., it will just be compared on item number). This ensures
+     * that lower priority items are not kept in the queue indefinitely if there is a constant
+     * stream of higher priority items.
+     *
+     * @param clock The clock to use to get the current time.
+     * @param starvationThreshold The time an item must be queued for to be considered starved.
+     * @return a comparator that compares items by their priority unless they are starved, and then
+     * their item number.
+     */
+    public static Comparator<QueueItem> byPriorityThenNumber(
+            final Clock clock,
+            final TemporalAmount starvationThreshold) {
+        return Comparator.<QueueItem, QueuePriority>comparing(
+                item -> getPriorityIfNotStarved(item, clock, starvationThreshold))
+                .thenComparing(QueueItem::getItemNumber);
+    }
+
+    /**
+     * Gets the priority of the item if it was created within the given timespan, or
+     * {@link QueuePriority#IMMEDIATE} if the item is older. This allows items that are otherwise
+     * being starved to be sorted to the front of a queue.
+     *
+     * @param item The item to get the priority of.
+     * @param clock The clock to use to get the current time.
+     * @param timespan The timespan after which an item will be considered starved.
+     * @return The priority of the item, or {@link QueuePriority#IMMEDIATE}.
+     */
+    private static QueuePriority getPriorityIfNotStarved(
+            final QueueItem item, final Clock clock, final TemporalAmount timespan) {
+        if (item.getTime().isAfter(LocalDateTime.now(clock).minus(timespan))) {
+            return item.getPriority();
+        } else {
+            return QueuePriority.IMMEDIATE;
+        }
+    }
+
+}

irc/src/com/dmdirc/parser/irc/outputqueue/QueueHandler.java

@@ -39,6 +39,8 @@ public abstract class QueueHandler extends Thread implements Comparator<QueueIte
     protected final BlockingQueue<QueueItem> queue;
     /** The output queue that owns us. */
     protected OutputQueue outputQueue;
+    /** Comparator to use to sort queue items. */
+    private final Comparator<QueueItem> comparator;
     /** Where to send the output. */
     private final PrintWriter out;
 
@@ -46,13 +48,19 @@ public abstract class QueueHandler extends Thread implements Comparator<QueueIte
      * Create a new Queue Thread.
      *
      * @param outputQueue the OutputQueue that owns us.
-     * @param queue Queue to handle
+     * @param queue Queue to handle.
+     * @param comparator Comparator to use to sort items in the queue.
      * @param out Writer to send to.
      */
-    public QueueHandler(final OutputQueue outputQueue, final BlockingQueue<QueueItem> queue, final PrintWriter out) {
+    public QueueHandler(
+            final OutputQueue outputQueue,
+            final BlockingQueue<QueueItem> queue,
+            final Comparator<QueueItem> comparator,
+            final PrintWriter out) {
         super("IRC Parser queue handler");
 
         this.queue = queue;
+        this.comparator = comparator;
         this.out = out;
         this.outputQueue = outputQueue;
     }
@@ -81,39 +89,9 @@ public abstract class QueueHandler extends Thread implements Comparator<QueueIte
         return new QueueItem(this, line, priority);
     }
 
-    /**
-     * Compare two QueueItems for sorting purposes.
-     * This is called by the default QueueItem in its compareTo method. The
-     * calling object will be the first parameter, the object to compare it to
-     * will be second.
-     * This allows QueueHandlers to sort items differently if needed.
-     *
-     * The default implementation works as follows:
-     * Compare based on priorty firstly, if the priorities are the same,
-     * compare based on the order the items were added to the queue.
-     *
-     * If an item has been in the queue longer than 10 seconds, it will not
-     * check its priority and soley position itself based on adding order.
-     *
-     * @param mainObject Main object we are comparing against.
-     * @param otherObject Object we are comparing to.
-     * @return A QueueItem for teh given parameters
-     */
     @Override
     public int compare(final QueueItem mainObject, final QueueItem otherObject) {
-        if (!isStarved(mainObject)
-                && mainObject.getPriority().compareTo(otherObject.getPriority()) != 0) {
-            return mainObject.getPriority().compareTo(otherObject.getPriority());
-        }
-
-        if (mainObject.getItemNumber() > otherObject.getItemNumber()) {
-            return 1;
-        } else if (mainObject.getItemNumber() < otherObject.getItemNumber()) {
-            return -1;
-        } else {
-            // This can't happen.
-            return 0;
-        }
+        return comparator.compare(mainObject, otherObject);
     }
 
     /**

irc/src/com/dmdirc/parser/irc/outputqueue/QueueItem.java

@@ -24,6 +24,7 @@ package com.dmdirc.parser.irc.outputqueue;
 
 import com.dmdirc.parser.common.QueuePriority;
 
+import java.time.Clock;
 import java.time.LocalDateTime;
 
 /**
@@ -52,11 +53,23 @@ public class QueueItem implements Comparable<QueueItem> {
      * @param priority Priority for the queue item
      */
     public QueueItem(final QueueHandler handler, final String line, final QueuePriority priority) {
+        this(handler, Clock.systemDefaultZone(), line, priority);
+    }
+
+    /**
+     * Create a new QueueItem.
+     *
+     * @param handler Handler for this QueueItem
+     * @param line Line to send
+     * @param priority Priority for the queue item
+     */
+    public QueueItem(final QueueHandler handler, final Clock clock, final String line,
+            final QueuePriority priority) {
         this.handler = handler;
         this.line = line;
         this.priority = priority;
 
-        this.time = LocalDateTime.now();
+        this.time = LocalDateTime.now(clock);
         this.itemNumber = number++;
     }
 

irc/src/com/dmdirc/parser/irc/outputqueue/SimpleRateLimitedQueueHandler.java

@@ -57,8 +57,11 @@ public class SimpleRateLimitedQueueHandler extends QueueHandler {
      * @param queue Queue to use
      * @param out Output Stream to use
      */
-    public SimpleRateLimitedQueueHandler(final OutputQueue outputQueue, final BlockingQueue<QueueItem> queue, final PrintWriter out) {
-        super(outputQueue, queue, out);
+    public SimpleRateLimitedQueueHandler(
+            final OutputQueue outputQueue,
+            final BlockingQueue<QueueItem> queue,
+            final PrintWriter out) {
+        super(outputQueue, queue, QueueComparators.byPriorityThenNumber(), out);
     }
 
     /**
@@ -163,21 +166,6 @@ public class SimpleRateLimitedQueueHandler extends QueueHandler {
         return isLimiting;
     }
 
-    /**
-     * Compare queue items, if priorities differ, then  higher priority items
-     * will always be put further ahead in the queue (This queue ignores the
-     * 10-second rule of the normal queue) otherwise the normal comparison is
-     * used.
-     */
-    @Override
-    public int compare(final QueueItem mainObject, final QueueItem otherObject) {
-        if (mainObject.getPriority().compareTo(otherObject.getPriority()) == 0) {
-            return super.compare(mainObject, otherObject);
-        } else {
-            return mainObject.getPriority().compareTo(otherObject.getPriority());
-        }
-    }
-
     @Override
     public QueueItem getQueueItem(final String line, final QueuePriority priority) {
         // Was the last line added less than limitTime ago?
@@ -186,7 +174,7 @@ public class SimpleRateLimitedQueueHandler extends QueueHandler {
             if (overTime) {
                 // If we are not currently limiting, and this is the items-th item
                 // added in the last limitTime, start limiting.
-                if (!isLimiting && ++count > (items - 1)) {
+                if (!isLimiting && ++count >= items) {
                     isLimiting = true;
                     count = 0;
                 }

irc/test/com/dmdirc/parser/irc/outputqueue/QueueComparatorsTest.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright (c) 2006-2016 DMDirc Developers
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package com.dmdirc.parser.irc.outputqueue;
+
+import com.dmdirc.parser.common.QueuePriority;
+
+import java.time.Clock;
+import java.time.Duration;
+import java.time.Instant;
+import java.time.ZoneId;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+public class QueueComparatorsTest {
+
+    private static final Clock NOW = Clock.fixed(Instant.ofEpochMilli(100 * 1000),
+            ZoneId.systemDefault());
+
+    private static final Clock NOW_MINUS_FIVE = Clock.fixed(Instant.ofEpochMilli(95 * 1000),
+            ZoneId.systemDefault());
+
+    private static final Clock NOW_MINUS_TEN = Clock.fixed(Instant.ofEpochMilli(90 * 1000),
+            ZoneId.systemDefault());
+
+    private final QueueItem currentLowPriority1 = new QueueItem(null, NOW_MINUS_FIVE,
+            "", QueuePriority.LOW);
+
+    private final QueueItem currentLowPriority2 = new QueueItem(null, NOW_MINUS_FIVE,
+            "", QueuePriority.LOW);
+
+    private final QueueItem currentNormalPriority = new QueueItem(null, NOW_MINUS_FIVE,
+            "", QueuePriority.NORMAL);
+
+    private final QueueItem currentHighPriority = new QueueItem(null, NOW_MINUS_FIVE,
+            "", QueuePriority.HIGH);
+
+    private final QueueItem oldLowPriority = new QueueItem(null, NOW_MINUS_TEN,
+            "", QueuePriority.LOW);
+
+    @Test
+    public void testComparesByPriority() {
+        assertEquals(0, QueueComparators.byPriorityThenNumber().compare(
+                currentLowPriority1,
+                currentLowPriority1));
+        assertTrue(QueueComparators.byPriorityThenNumber().compare(
+                currentNormalPriority,
+                currentLowPriority2) < 0);
+        assertTrue(QueueComparators.byPriorityThenNumber().compare(
+                currentHighPriority,
+                oldLowPriority) < 0);
+        assertTrue(QueueComparators.byPriorityThenNumber().compare(
+                currentNormalPriority,
+                currentHighPriority) > 0);
+    }
+
+    @Test
+    public void testComparesByNumber() {
+        // If the priority is the same, then the lowest number comes first.
+        assertTrue(QueueComparators.byPriorityThenNumber().compare(
+                currentLowPriority1,
+                currentLowPriority2) < 0);
+        assertTrue(QueueComparators.byPriorityThenNumber().compare(
+                currentLowPriority2,
+                currentLowPriority1) > 0);
+    }
+
+    @Test
+    public void testStarvedItemsFirst() {
+        // If an item is starved then it gets queued first
+        assertTrue(QueueComparators.byPriorityThenNumber(NOW, Duration.ofSeconds(7)).compare(
+                oldLowPriority,
+                currentHighPriority) < 0);
+        assertTrue(QueueComparators.byPriorityThenNumber(NOW, Duration.ofSeconds(7)).compare(
+                currentLowPriority1,
+                oldLowPriority) > 0);
+    }
+
+}