Re: [cp-patches] Re: Add JSpinner and friends added

[graydon hoare]

On Sun, 22 Aug 2004 15:59:25 -0700, Ka-Hing Cheung <address@hidden> wrote:

> Okay, here's the Timer patch as promised (only about 2 weeks late! I am
> getting better).
>
> This is more or less the same as the last one I sent in, except that
> this one behaves more correctly when it's started/stopped really quick.
> It also lets the thread die when there's no timer running. Although it
> seems like SwingUtilities.invokeLater creates the event dispatch thread,
> but the thread keeps running even though no Swing component is ever
> created. So if I create a javax.swing.Timer and no Swing components,
> the program will never terminate. This is with gcj, I have no tried
> other VMs, but I suspect they would be the same.

hi,

I've got a couple concerns with this patch. it may be that I just
don't understand what you're doing, but if you could clarify them
I'd appreciate it:

   protected void fireActionPerformed(ActionEvent event)
   {
-    ActionListener[] listeners = getActionListeners();
+    final ActionEvent evt = event;
+    final ActionListener[] listeners = getActionListeners();

-    for (int i = 0; i < listeners.length; i++)
-      listeners[i].actionPerformed(event);
+    synchronized(this)
+      {
+        for (int i = 0; i < listeners.length; i++)
+          {
+            final int which = i;
+            if(!(eventQueued && isCoalesce()))
+              {
+                eventQueued = true;
+                SwingUtilities.invokeLater(new Runnable()
+                  {
+                    public void run()
+                    {
+                      listeners[which].actionPerformed(evt);
+                      eventQueued = false;
+                    }
+                  });
+              }
+          }
+      }
   }

this method appears to queue up an async callback from the event
dispatching thread which will reach back into the Timer and
switch the eventQueued flag to false. this seems like it will
race with the outer method; it might be harmless given that the
flag only controlls coalescing, but it strikes me as potentially
incorrect. at very least, I don't know what would happen if you
were in non-coalescing mode, queued up a dozen events, then switched
to coalescing mode and continued to try to queue new events at high
speed. it's not clear to me that the situation would ever reduce to
a single queued event since queued events might keep setting
eventQueued to false after they fire.

+  private boolean repeat = true;

minor nit: I think if the property name is "repeats" then the field
modelling it ought to be "repeats" not "repeat". but the real issue
I'm worried about is:

+          try
+            {
+              sleep(50);
+            }
+          catch (InterruptedException _) {}

I wonder why this is not doing:

  Timer.class.wait(t.nextExecTime - System.currentTimeMillis())

since it seems like we will have a busy-wait at 50 msec
granularity, with the proposed code.

-graydon