Bug 338559

Comment 1 Thomas Schindl 2011-03-01 11:58:30 EST

Created attachment 190057 [details]
patch

Comment 2 Oleg Besedin 2011-03-03 11:20:40 EST

With the patch I am getting 120+ test failures when I run UIAllTests JUnits. Is it because Realm is not set in the context?

Comment 3 Thomas Schindl 2011-03-03 11:24:46 EST

I only know that the Workbench-Code sets a realm. I need to see why this fails. maybe the reason is that it is now asyncExec instead of syncExec. The change is the same we made in the DI-Code

Comment 4 Oleg Besedin 2011-03-03 11:25:16 EST

You replacing 
   Display.getDefault().syncExec()
with
   realm.exec()

The realm.exec() translates into async call for non-UI thread. So, in practice,
you are changing code to execute non-main thread calls as async calls.

Comment 5 Thomas Schindl 2011-03-03 11:26:48 EST

Correct. This is something i mentioned in the mail to e4-dev. I think what we should add is an abstraction for Display#syncExec/asyncExec because so this API is toolkit neutral.

Comment 6 Thomas Schindl 2011-03-03 11:35:14 EST

The API i have in mind is something like this:

interface UISync {
  public void syncExec(Runnable runnable);
  public void asyncExec(Runnable runnable);
}

Comment 7 Thomas Schindl 2011-03-03 12:05:11 EST

Created attachment 190287 [details]
Abstracting sync/asynExec

This brings me down to on test failure but I doubt the failing test is because of this change

Comment 8 Thomas Schindl 2011-03-03 12:06:27 EST

Created attachment 190288 [details]
now with all changes

Comment 9 Thomas Schindl 2011-03-03 12:18:01 EST

Ok the test failure is expected and tracked in bug 333869

Comment 10 Oleg Besedin 2011-03-03 14:48:51 EST

(In reply to comment #8)
> Created attachment 190288 [details]
> now with all changes

Looks good! You might consider a few small adjustments:

- UISynchronize: if we expect other people to implement this, better to make it an abstract class instead of an interface - more flexible
- UISynchronize needs some Javadoc for the interface itself
- UISynchronize implementation in the E4Application: I am not sure if it makes sense to put a guard on (display == null) in there. It is probably not possible to get to that place with null display today, but somehow I have a feeling that if the same code will be reached in a headless run, that might happen
- Update years on copyright headers

Comment 11 Thomas Schindl 2011-03-03 15:24:43 EST

Done beside adding the null check which is impossible be null at this point