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

[Ka-Hing Cheung]

On Thu, 2004-09-09 at 13:06, graydon hoare wrote:
> 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:

Somehow I missed this email w/o Mark pointing me to 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.

I suppose this can be solved by putting a synchronized block in
setCoalesce()?

> 
> +  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

hmm good point.

> 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.

Mainly because I didn't think of it. With this I will also need to make
the call to notify() in putPendingTimer unconditional.

I need to head out in a few, but I will submit a new patch tonight if
there's no further objections.

-khc