Bug 246929 - SWT_AWT.new_Shell not working in Cocoa
Summary: SWT_AWT.new_Shell not working in Cocoa
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.5   Edit
Hardware: Macintosh Mac OS X
: P3 normal with 28 votes (vote)
Target Milestone: 3.6 M3   Edit
Assignee: Scott Kovatch CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-10 19:06 EDT by Scott Kovatch CLA
Modified: 2009-10-26 12:29 EDT (History)
24 users (show)

See Also:


Attachments
Simple test case of the SWT browser in a Swing UI. (1.89 KB, text/x-java)
2009-09-18 15:01 EDT, Christopher Deckers CLA
no flags Details
Example where Display is not created in the main thread (2.04 KB, text/x-java)
2009-09-19 04:01 EDT, Christopher Deckers CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Kovatch CLA 2008-09-10 19:06:10 EDT
As of 9/9, SWT_AWT.new_Frame() is working in the Cocoa port. See bug #245724 for the last issue there.

I added an implementation of new_Shell() as well. It doesn't crash, and appears to add controls to the NSView that represents the embedded shell, but they aren't drawing.

This is a bug to remind me to finish it up before 3.5 is finished.
Comment 1 Scott Kovatch CLA 2008-09-10 19:11:57 EDT
Taking this, since I'm working on it.
Comment 2 Silenio Quarti CLA 2008-09-15 12:28:06 EDT
We looked at the partial implementation that you committed. We saw bunch of problems.

In Shell, when SWT is embedded in AWT, the window field is null.  There are a bunch of places (for example Shell.setAlpha()) where get the view's window.  It looks to us like calling setAlpah() would set alpha on the AWT frame, not the embedded SWT frame.  Other places include getLocation(), computeTrim() (that uses the AWT frame trim) etc.

There is a direct reverence to window from Menu._setVisible().  That one will cause a null pointer exception.

We believe that the right pattern should be a test for "window == null" and run different code rather than getting the window from the view.

We were looking at the "isEmbedded" field in Display.  First off, there won't be any mouse events anywhere because we aren't subclassing NSApplication.  The biggest problem we have with the use of the embedded field is that is makes the toolkit free threaded.

What do you think?  Steve thinks that we should either fix the code or make a patch to capture the current state, revert a previous state without the code.  When we address this bug report, the patch will not be useful directly, but it will capture work.

Alternately, it could all be left in with isEmbedded always false and the "not implemented" exception but back.

Finally, I suppose we could just keep going and ignore it, but normally we wouldn't do this.

Silenio & Steve
Comment 3 Scott Kovatch CLA 2008-09-15 13:34:57 EDT
(In reply to comment #2)
> We looked at the partial implementation that you committed. We saw bunch of
> problems.
> 
> In Shell, when SWT is embedded in AWT, the window field is null.  There are a
> bunch of places (for example Shell.setAlpha()) where get the view's window.  It
> looks to us like calling setAlpah() would set alpha on the AWT frame, not the
> embedded SWT frame.  Other places include getLocation(), computeTrim() (that
> uses the AWT frame trim) etc

> There is a direct reverence to window from Menu._setVisible().  That one will
> cause a null pointer exception.
> 
> We believe that the right pattern should be a test for "window == null" and run
> different code rather than getting the window from the view.

There are times when it is correct to get the view's window and times when it is not.  For example, I agree that setAlpha/getAlpha should not change the alpha of the owning window.

> We were looking at the "isEmbedded" field in Display.  First off, there won't
> be any mouse events anywhere because we aren't subclassing NSApplication.  The
> biggest problem we have with the use of the embedded field is that is makes the
> toolkit free threaded.

Mouse events will be delivered to you by the AWT's NSApplication instance, which is pumping events on thread 0.  However, based on our conversation on Friday about the SWT's intercepting of mouse events first, you are right that you won't get to intercept and generate SWT mouse events because you won't have control over the NSApplication.  I don't know how to address that.

Without the isEmbedded field and checks for it during creation of the Display, you'll get multiple Apple menus.  We'll also have the problem of having two competing loops attempting to read and dispatch events.

As far as the free-threaded issue goes, I was concerned about putting that in too, but without it, embedded Shells are almost guaranteed not to work as most AWT calls are not going to happen on the same thread that created the display.
I may have missed something on the design of Shell embedding, in which case I made a mistake by adding that check.

After some more thought, I don't know how this will work right now, because like Carbon, Cocoa calls can't happen on the non-main thread either. Every Cocoa method we invoke would need to be wrapped inside a performSelectorOnMainThread to ensure that no Cocoa operation from the AWT is happening at the same time as the SWT.

> What do you think?  Steve thinks that we should either fix the code or make a
> patch to capture the current state, revert a previous state without the code. 
> When we address this bug report, the patch will not be useful directly, but it
> will capture work.
>
> Alternately, it could all be left in with isEmbedded always false and the "not
> implemented" exception but back.

As I mentioned earlier, this code was written before we had the discussion about how events are currently being generated in the SWT.  A lot of this code is going to be incompatible with that, so I think we'll need to put back the notImplemented part of the code.

I do think the isEmbedded should stay there as a reminder, with some comments on the issues that need addressing.
Comment 4 Scott Kovatch CLA 2008-09-15 14:06:25 EDT
I'm going to comment Display and Shell and remove some of the more sweeping changes.  For now I'm also going to disable new_Shell, as there are some architectural-level issues that will need to be worked out.
Comment 5 Antoine Toulmé CLA 2008-12-16 09:26:37 EST
Is this bug the reason why no preference dialog shows up when clicking on Preferences in the 3.5M4 build ?

This is the only thing that forbids me to switch to the Cocoa port from the testing I was able to conduct (great work so far!).
Comment 6 Kevin Barnes CLA 2008-12-16 09:33:09 EST
Antoine see Bug 258113.
Comment 7 Christopher Deckers CLA 2009-03-24 09:03:38 EDT
Is a fix still planned for 3.5 or is it going to be postponed?
Comment 8 Scott Kovatch CLA 2009-03-30 19:18:20 EDT
(In reply to comment #7)
> Is a fix still planned for 3.5 or is it going to be postponed?

After talking to Mike S. at Apple last week I think I have an idea of how it could be done using the AWT CocoaComponent, which handles AppKit being called off the main thread.

I need to juggle this work with my main job's needs, however.  I hope to have this done by 3.5 final, but I can't promise anything right now.
Comment 9 Raji Akella CLA 2009-06-23 13:49:33 EDT
Hi Scott, what is the latest on this?

Comment 10 Harold CLA 2009-07-09 16:02:34 EDT
(In reply to comment #9)
> Hi Scott, what is the latest on this?
> 

I would like to know status too. Thanks!
Comment 11 Scott Kovatch CLA 2009-07-09 16:22:11 EDT
It may be a while before I can get back to this.  Flash Catalyst is in the final drive to 1.0, so SWT work for me is on hold until October at the earliest.
Comment 12 david wafula CLA 2009-08-02 18:17:29 EDT
hi,
eagerly waiting for this fix !..stopped some project :) thanks :)
Comment 13 Patrick Talbot CLA 2009-08-25 23:26:44 EDT
(In reply to comment #8)
> After talking to Mike S. at Apple last week I think I have an idea of how it
> could be done using the AWT CocoaComponent, which handles AppKit being called
> off the main thread.
> I need to juggle this work with my main job's needs, however.  I hope to have
> this done by 3.5 final, but I can't promise anything right now.

Hi Scott,
did you find some time to address this problem? Or maybe someone else could assist if you could give some pointers on your idea?
This is really a major problem and it really is affecting the adoption of SWT on Mac OS.
Please give us some feedback. Thanks!
Comment 14 Harold CLA 2009-08-26 12:18:57 EDT
(In reply to comment #13)
> Hi Scott,
> did you find some time to address this problem? Or maybe someone else could
> assist if you could give some pointers on your idea?
> This is really a major problem and it really is affecting the adoption of SWT
> on Mac OS.
> Please give us some feedback. Thanks!
> 

I worked around this problem by creating a JNI library to open a NSOpenPanel. It took some time to learn XCode but finally got it working the way I need it too. Here's the sample project that I started with: http://developer.apple.com/samplecode/JSheets/index.html
Comment 15 Scott Kovatch CLA 2009-08-26 15:12:11 EDT
Here are some notes I sent to Silenio today. It will be early September before I am working on SWT again.

"We need to talk about how the NSApplication subclass works with the AWT.  In short, there will already be an NSApplication instance up and running by the time something wants to embed SWT in AWT, so we either need to work with the one that's already there or subclass it on the fly, similar to what we were doing with drag and drop.

After that I think the only way we can do this is with an NSView that represents an SWT Shell inside a CocoaComponent. See:

http://developer.apple.com/documentation/Java/Reference/1.5.0/appledoc/api/com/apple/eawt/CocoaComponent.html

There's an example at http://developer.apple.com/samplecode/CWCocoaComponent/  that shows how to make an NSColorWell, but we should be able to extend this to SWT_AWT embedding pretty easily."
Comment 16 Silenio Quarti CLA 2009-08-26 16:10:51 EDT
(In reply to comment #15)
> Here are some notes I sent to Silenio today. It will be early September before
> I am working on SWT again.
> "We need to talk about how the NSApplication subclass works with the AWT.  In
> short, there will already be an NSApplication instance up and running by the
> time something wants to embed SWT in AWT, so we either need to work with the
> one that's already there or subclass it on the fly, similar to what we were
> doing with drag and drop.

I think there two cases: It is an SWT app that creates an AWT frame and then embedded SWT in that frame, or it is an AWT app (event loop). In either case, we should be able to subclass the class of the shared NSApplication instance on the fly.

In the second case, I am not sure what to do with Display.readAndDispatch(), since AWT will be running the only event loop.




Comment 17 HJK CLA 2009-09-11 03:19:36 EDT
I am waiting eagerly for this bug to be solved....
Comment 18 ian.cordingley CLA 2009-09-11 11:44:22 EDT
This bug is causing significant challenges with our Mac development. Is there likely to be a rapid resolution to this one???

This really is a huge road-block to multi-platform deployment because of the Mac SWT Issue!
Comment 19 karelapple CLA 2009-09-15 03:21:13 EDT
It definetly would be very, very great if this issue could be fixed asap :-)
Thanks in advance!
Comment 20 Scott Kovatch CLA 2009-09-15 12:39:15 EDT
I think the swine flu took me out of commission last week, but I hope to be getting back to this soon.

(In reply to comment #16)
> I think there two cases: It is an SWT app that creates an AWT frame and then
> embedded SWT in that frame, or it is an AWT app (event loop). In either case,
> we should be able to subclass the class of the shared NSApplication instance on
> the fly.

Correct, and I think we're now set with the AWT-SWT sharing/subclassing of NSApplication. All of the fixed Web Start bugs demonstrate that.
 
> In the second case, I am not sure what to do with Display.readAndDispatch(),
> since AWT will be running the only event loop.

Yes, the separation of AWT and SWT activity will become even more important. All Cocoa activity (and by extension, SWT) must be done on the main thread, but all AWT activity must happen on a non-main thread. For readAndDispatch, we may have to check which thread we are on and just quietly do nothing, or block somehow, if called from the non-main thread.

Right now I'm thinking that AWT apps will need to make heavy use of CocoaComponent when attempting to work with SWT components.
Comment 21 Silenio Quarti CLA 2009-09-15 13:52:16 EDT
Wow! Hope you are feeling better.
Comment 22 Christopher Deckers CLA 2009-09-18 15:01:33 EDT
Created attachment 147590 [details]
Simple test case of the SWT browser in a Swing UI.
Comment 23 Christopher Deckers CLA 2009-09-19 04:01:39 EDT
Created attachment 147630 [details]
Example where Display is not created in the main thread

This test case does not create a Display in the main thread, which can be important for integration in frameworks (like plugin frameworks) which do not let you control the main method.

This works on Windows and Linux, but if I remember correctly Mac does not allow creating a Display from a different thread than the main thread (so this is not an SWT-AWT issue). At least, the code example is captured here for future reference.
Comment 24 Scott Kovatch CLA 2009-09-21 13:25:51 EDT
Fixed > 20090921.

Testing was based mainly on the first example here. I will add some additional checking for the display-created-on-non-main-thread case to at least do something predictable.
Comment 25 Scott Kovatch CLA 2009-09-21 13:35:22 EDT
(In reply to comment #23)
> This test case does not create a Display in the main thread, which can be
> important for integration in frameworks (like plugin frameworks) which do not
> let you control the main method.

Chris, can you give an example of where this would happen? Is this something that would come up in NetBeans?
Comment 26 Christopher Deckers CLA 2009-09-21 13:46:53 EDT
(In reply to comment #25)
> Chris, can you give an example of where this would happen? Is this something
> that would come up in NetBeans?

I don't know the specifics of NetBeans, but I would guess so. Most plugin frameworks do not expose their main method, which prevents us from adding the Display creation and event pumping.
For example, I am currently discussing this particular Mac integration issue in the context of a framework (called Servoy) where this does not seem to be possible (and of course, portability to Mac seems to be a big showstopper for some users).
Comment 27 Silenio Quarti CLA 2009-09-21 17:41:46 EDT
Scott, I have a few of questions:

1) do we still need to check display.isEmbedded in Widget.checkWidget()? I believe it is unnecessary since the display is always created in the main thread.

2) Shell.getBounds() does not do the same kind of work that Shell.getLocation() does to calculate x and y. getLocation() returns the location relative to the window and getBounds() returns it relative to the parent view. Also the userSpaceScaleFactor is not used in getBounds().

3) Why do we have to retain() the window.contentView() in Shell.createHandle()? It looks like we are leaking. Note that the window content window is set in Shell.setZOrder().

4) I believe key events will not happen in single text, combo and spinner widgets if this code does not happen in Shell.createHandle():

                id id = window.fieldEditor (true, null);
		if (id != null) {
			OS.object_setClass (id.id, OS.objc_getClass ("SWTEditorView"));
		}
Comment 28 Scott Kovatch CLA 2009-09-21 18:47:05 EDT
(In reply to comment #27)
> 1) do we still need to check display.isEmbedded in Widget.checkWidget()? I
> believe it is unnecessary since the display is always created in the main
> thread.

I added that check some time ago -- if we can enforce that restriction, yes, it should be removed. I will double-check that new_Frame() isn't depending on that behavior.

> 2) Shell.getBounds() does not do the same kind of work that Shell.getLocation()
> does to calculate x and y. getLocation() returns the location relative to the
> window and getBounds() returns it relative to the parent view. Also the
> userSpaceScaleFactor is not used in getBounds().

After rereading the contract of getBounds, I see now I need to return a Rectangle in global coordinates in the embedded case. I'll fix that.

> 3) Why do we have to retain() the window.contentView() in Shell.createHandle()?
> It looks like we are leaking. Note that the window content window is set in
> Shell.setZOrder().

We only need to retain() when the state has the FOREIGN_HANDLE bits set. This is confusing, because the 'view' member variable represents something else in the embedded case until super.createHandle() gets called. If the window already has a contentView, then we don't want to create another one, as we don't want to overwrite the content view of a window we didn't create.

Control.releaseHandle() calls view.release(), so if we don't create a new content view we have to retain() it or else it would be over-released when the host window disposes of the content view.  I'll add some extra comments to clear this up.

> 4) I believe key events will not happen in single text, combo and spinner
> widgets if this code does not happen in Shell.createHandle():
> 
>                 id id = window.fieldEditor (true, null);
>         if (id != null) {
>             OS.object_setClass (id.id, OS.objc_getClass ("SWTEditorView"));
>         }

Okay, I will check this. But, simply setting the class of the field editor may be a problem if the AWT already has its own field editor class, which I believe it does. If the top-level window has both an AWT TextField and an SWT Text, the AWT may lose text events. If we have problems I will open a new bug.

So, Q1 and 2 are fixes I need to make. Q4 is as well, but I hope it won't be a huge problem. For Q3, I think the code is correct as is, but I will add more comments to explain the logic.
Comment 29 Scott Kovatch CLA 2009-09-21 19:27:01 EDT
Updated with comments after code review (comment #27).
Comment 30 Kevin Barnes CLA 2009-09-22 12:51:24 EDT
fixed resolution independence issues in getBounds.