<AWT Dev> hg: jdk7/awt/jdk: 6949936: Provide API for running nested events loops, similar to what modal dialogs do

[Rémi Forax]

Le 25/08/2010 13:00, Artem Ananiev a écrit :
> Hi, Rémi,

Hi Artem,

>
> thanks for your feedback. See more comments below.
>
> On 8/24/2010 2:36 PM, Rémi Forax wrote:
>> Le 24/08/2010 10:57, artem.ananiev at sun.com a écrit :
>>> Changeset: 7e26538596be
>>> Author: art
>>> Date: 2010-08-24 12:54 +0400
>>> URL: http://hg.openjdk.java.net/jdk7/awt/jdk/rev/7e26538596be
>>>
>>> 6949936: Provide API for running nested events loops, similar to what
>>> modal dialogs do
>>> Reviewed-by: ant, anthony
>>>
>>> ! src/share/classes/java/awt/Dialog.java
>>> ! src/share/classes/java/awt/EventDispatchThread.java
>>> ! src/share/classes/java/awt/EventQueue.java
>>> + src/share/classes/java/awt/SecondaryLoop.java
>>> + src/share/classes/java/awt/WaitDispatchSupport.java
>>> + test/java/awt/EventQueue/SecondaryLoopTest/SecondaryLoopTest.java
>>>
>>
>> Hi all, nice to see that this kind of API find its way to jdk7.
>>
>> The example provided in SecondaryLoop don't compile.
>> Here is a slightly modified version that compile.
>
> The code in JavaDoc is not a complete test case, but rather a kind of 
> stub. To make it compile, you need to declare 'loop' final or make it 
> a class field, put the code with ActionListener into a method, etc.

Why not providing an example that compile ?

>
>> JButton jButton = new JButton("Button");
>> jButton.addActionListener(new ActionListener() {
>> @Override
>> public void actionPerformed(ActionEvent e) {
>> EventQueue queue = Toolkit.getDefaultToolkit()
>> .getSystemEventQueue();
>> final SecondaryLoop loop = queue.createSecondaryLoop();
>>
>> Runnable runnable = new Runnable() {
>> @Override
>> public void run() {
>> // Perform calculations
>> doSomethingUseful();
>>
>> // Exit the loop
>> loop.exit();
>> }
>> };
>>
>> // Spawn a new thread to do the work
>> Thread worker = new Thread(runnable);
>> worker.start();
>>
>> // Enter the loop to block the current event
>> // handler, but leave UI responsive
>> if (!loop.enter()) {
>> // Report an error
>> }
>> }
>> });
>>
>> About the API design, SecondaryLoop.enter is not reentrant so it's
>> an error to call it recursively. So enter() should throw a runtime
>> exception
>> instead of returning false. I propose to use IllegalStateException.
>
> Well, these two approaches are roughly the same, but throwing an 
> exception usually makes the code a bit less clear.

I don't understand your point here, you propose:
if (!loop.enter) {
    // you have to do something useful here !!
}

But with a runtime exception you have just:
loop.enter();

Moreover, people are lazy, you can't force them to use return value.
In C, you know that this is bad, but in Java this is ok because if you use
an API in a wrong way, you use to receive an exception.

Also note that is exception is runtime, so you don't force user to catch it,
because they should not catch it but change their code to don't call
recursively enter().


>
>> SecondaryLoop is in my opinion a bad name, a for or a while is a loop,
>> in my opinion, SecondaryEventLoop is a better name.
>>
>> Also, you don't provide a way to transfer any value from the computation
>> done in the thread back to the event queue thread like by example
>> the SwingWorker does.
>>
>> I propose to change SecondaryEventLoop to:
>>
>> public interface SecondaryEventLoop<V> {
>> public V enter();
>> public boolean exit(V value);
>> }
>>
>> exit() can send a value that will be returned when enter() will end.
>>
>> I also wonder if the return value of exit is useful.
>> exit can be called before enter() due to a race between enter() and 
>> exit(),
>> why a developer should care about that.
>
> SecondaryLoop is different from SwingWorker: it shouldn't be used to 
> perform a task and obtain a value. 

Why ?
You create APIs, and often people use it in a slightly different way 
than your original through.

The swing worker works in an 'asymetric' way, by example if you want to 
disable
a button and re-enable it at the end of a background task, you will 
write something
like that with a swing worker:

jButton.addActionListener(new ActionListener() {
   @Override
   public void actionPerformed(ActionEvent e) {
     jButton.setEnable(false);

     new SwingWorker<Void,Void>() {
       protected Void doInBackGround() {
          // the background task
       }
       protected void done() {
          jButton.setEnable(true);
       }
     }.start();
   }
};

You can notice that the setEnable(true) is not done in the action listener.

The secondary event loop can be seen as a more 'symetric' way to perform
such task.

jButton.addActionListener(new ActionListener() {
   @Override
   public void actionPerformed(ActionEvent e) {
     jButton.setEnable(false);
     final SecondaryLoop loop = Toolkit.getDefaultToolkit()
         .getSystemEventQueue().createSecondaryLoop();

     new Thread(new Runnable() {
       protected void run() {
          // the background task

          loop.exit();
       }
     }.start();

     loop.enter();
     jButton.setEnable(true);
   }
};

Here, the code is more symetric because setEnable(true) is done in the 
action listener.
If the SecondaryEventLoop API exists, people will use it :)

Now, suppose that the code below exists somewhere in a codebase and
that the use case change and the value computed by the backgound task
need to be displayed on the screen.
Any duke will not use the swing worker here but try to adapt the code and
there is lot of chance that he will end up with a non thread safe code.

> Its only purpose is to make sure UI is not frozen (EDT keeps running 
> even if the current event handler is blocked), but it doesn't spawn 
> any new threads and doesn't delegate any work to other threads.

Yes, but you can spawn thread easily in Java.

My main concern is that if you don't provide a way to get the resulting 
value,
people we workaround that but in a non-thread safe way.
I think it's better to provide an API that allows to return a value in a 
proper way
instead of having to write blogs to explain after what is the correct way
to do that in a thread safe way.

>
>> cheers,
>> Rémi
>
> Thanks,
>
> Artem

regards,
Rémi