Fix concurrency problems in OSD plugin

Change-Id: Iae88af8028e55d45540c4272324dd792233a5d91
Reviewed-on: http://gerrit.dmdirc.com/651
Reviewed-by: Shane Mc Cormack <shane@dmdirc.com>
Automatic-Compile: Gregory Holmes <greg@dmdirc.com>
Reviewed-by: Gregory Holmes <greg@dmdirc.com>

src/com/dmdirc/addons/osd/OsdManager.java

@@ -83,10 +83,15 @@ public class OsdManager {
 
     /**
      * Create a new OSD window with "message".
+     * <p>
+     * This method needs to be synchronised to ensure that the window list is
+     * not modified in between the invocation of {@link #getYPosition()} and
+     * the point at which the {@link OSDWindow} is added to the windowList.
      *
+     * @see #getYPosition()
      * @param message Text to display in the OSD window.
      */
-    private void displayWindow(final String message) {
+    private synchronized void displayWindow(final String message) {
         windowList.add(UIUtilities.invokeAndWait(
                 new ReturnableThread<OsdWindow>() {
 
@@ -97,7 +102,8 @@ public class OsdManager {
                         IdentityManager.getGlobalConfig().getOptionInt(
                         plugin.getDomain(), "locationX"), getYPosition(),
                         plugin, OsdManager.this));
-            }}));
+            }
+        }));
     }
 
     /**
@@ -106,38 +112,37 @@ public class OsdManager {
      *
      * @param window The window that we are destroying.
      */
-    public void closeWindow(final OsdWindow window) {
+    public synchronized void closeWindow(final OsdWindow window) {
         final String policy = IdentityManager.getGlobalConfig().getOption(
                 plugin.getDomain(), "newbehaviour");
 
-        synchronized (OsdManager.this) {
-            UIUtilities.invokeAndWait(new Runnable() {
-                /** {@inheritDoc} */
-                @Override
-                public void run() {
-                    int oldY = window.getY();
-                    final int closedIndex = windowList.indexOf(window);
+        int oldY = window.getDesiredY();
+        final int closedIndex = windowList.indexOf(window);
 
-                    if (closedIndex == -1) {
-                        return;
-                    }
+        if (closedIndex == -1) {
+            return;
+        }
 
-                    windowList.remove(window);
-                    window.dispose();
+        windowList.remove(window);
 
-                    final List<OsdWindow> newList = getWindowList();
+        UIUtilities.invokeLater(new Runnable() {
+            /** {@inheritDoc} */
+            @Override
+            public void run() {
+                window.dispose();
+            }
+        });
 
-                    for (OsdWindow otherWindow : newList.subList(closedIndex, newList.size())) {
-                        final int currentY = otherWindow.getY();
+        final List<OsdWindow> newList = getWindowList();
+        for (OsdWindow otherWindow : newList.subList(closedIndex, newList.size())) {
+            final int currentY = otherWindow.getDesiredY();
 
-                        if ("down".equals(policy) || "up".equals(policy)) {
-                                otherWindow.setLocation(otherWindow.getX(), oldY);
-                                oldY = currentY;
-                        }
-                    }
-                }
-            });
+            if ("down".equals(policy) || "up".equals(policy)) {
+                otherWindow.setDesiredLocation(otherWindow.getDesiredX(), oldY);
+                oldY = currentY;
+            }
         }
+
         displayWindows();
     }
 
@@ -170,6 +175,13 @@ public class OsdManager {
 
     /**
      * Get the Y position for the next window.
+     * <p>
+     * In order to ensure that windows are displayed at the correct position,
+     * the calling party MUST ensure that the window list is not altered between
+     * this method's invocation and the time at which the window is displayed.
+     * If the window list is altered, multiple windows may appear on top of
+     * each other instead of stacking correctly, or there may be gaps in up/down
+     * policy layouts.
      *
      * @return the Y position for the next window.
      */

src/com/dmdirc/addons/osd/OsdWindow.java

@@ -24,6 +24,7 @@ package com.dmdirc.addons.osd;
 
 import com.dmdirc.Main;
 import com.dmdirc.addons.ui_swing.MainFrame;
+import com.dmdirc.addons.ui_swing.UIUtilities;
 import com.dmdirc.config.IdentityManager;
 import com.dmdirc.ui.messages.ColourManager;
 
@@ -70,6 +71,9 @@ public final class OsdWindow extends JDialog implements MouseListener,
     
     /** Starting positions of the mouse. */
     private int startX, startY;
+
+    /** Desired position. */
+    private volatile int desiredX, desiredY;
     
     /** Is this a config instance? */
     private final boolean config;
@@ -99,6 +103,9 @@ public final class OsdWindow extends JDialog implements MouseListener,
 
         setDefaultCloseOperation(WindowConstants.DISPOSE_ON_CLOSE);
 
+        desiredX = x;
+        desiredY = y;
+
         setLocation(x, y);
 
         panel = new JPanel();
@@ -124,8 +131,8 @@ public final class OsdWindow extends JDialog implements MouseListener,
         pack();
 
         if (config) {
-            this.addMouseMotionListener(this);
-            this.addMouseListener(this);
+            addMouseMotionListener(this);
+            addMouseListener(this);
         } else {
             addMouseListener(this);
             new Timer("OSD Display Timer").schedule(new TimerTask() {
@@ -255,8 +262,63 @@ public final class OsdWindow extends JDialog implements MouseListener,
         }
     }
 
+    /**
+     * Retrieves the desired x offset of this OSD window.
+     *
+     * @since 0.6.3
+     * @see #setDesiredLocation(int, int)
+     * @return The desired offset of this window
+     */
+    public int getDesiredX() {
+        return desiredX;
+    }
+
+    /**
+     * Retrieves the desired y offset of this OSD window.
+     *
+     * @since 0.6.3
+     * @see #setDesiredLocation(int, int)
+     * @return The desired offset of this window
+     */
+    public int getDesiredY() {
+        return desiredY;
+    }
+
+    /**
+     * Sets the desired location of this OSD window, and queues an event to
+     * move the window to the desired location at some point in the future.
+     * <p>
+     * This method WILL NOT alter the location immediately, but will schedule
+     * an event in the AWT event despatch thread which will be executed in
+     * the future.
+     * <p>
+     * This method will immediately update the values returned by the
+     * {@link #getDesiredX()} and {@link #getDesiredY()} methods, but the
+     * {@link #getX()} and {@link #getY()} methods will continue to reflect the
+     * actual location of the window.
+     * <p>
+     * This method is thread safe.
+     *
+     * @param x The desired x offset of this window
+     * @param y The desired y offset of this window
+     * @since 0.6.3
+     */
+    public void setDesiredLocation(final int x, final int y) {
+        this.desiredX = x;
+        this.desiredY = y;
+
+        UIUtilities.invokeLater(new Runnable() {
+            /** {@inheritDoc} */
+            @Override
+            public void run() {
+                setLocation(getDesiredX(), getDesiredY());
+            }
+        });
+    }
+
     /** {@inheritDoc} */
-    @Override  public String toString() {
+    @Override
+    public String toString() {
         return label.getText();
     }
 }