Bugzilla – Full Text Bug Listing
| Summary: | Reparenting support on MacOS X | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Alexey Maslov <amaslov> | ||||||||
| Component: | SWT | Assignee: | Silenio Quarti <Silenio_Quarti> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | aramis, bill, bradleyjames, Dominique.Buenzli, eclipse, emoffatt, fredwulff, mg_list, oli, rszhang, Silenio_Quarti, snorthov, sxenos, wuhaijie | ||||||||
| Version: | 3.1 | Keywords: | helpwanted | ||||||||
| Target Milestone: | 3.3 M6 | ||||||||||
| Hardware: | Macintosh | ||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 56793, 70050 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alexey Maslov
This is a bunch of work and tricky to get right on this platform. Use of eparenting is generally considered bad UI design (there are notable exceptions). Can you rewrite your code to avoid Control.setParent()? Avoiding setParent() is pretty hard as we have designed our own framework on top of SWT that's pretty close to Swing in terms of parent/child relationship design. Each container has add() method to add children. Maybe our design is wrong? Please, explain why 'use of reparenting considered bad design'? And what are 'notable exceptions'? First let's look at the add() method. Given that a control can only have a single parent, what happens when the control is added in 2 different parents? Given that a control can't be drawn or really used without a parent, when does it ever make sense to create a control and then never give it a parent? So, the fact that the parent must be included in the constructor (a restriction imposed by most native window systems), while somewhat strange, is key in the design of SWT. So, controls should always have parents. If you allow reparenting, what this means is that anybody anywhere inside SWT or in client code, who stores the parent of a control in a field can have that parent changed. This leads to incredibly sublte bugs (Windows has a bunch involving hover help and the tool bar). One notable exception is the "tear off" case where you want to move a widget hierarchy to a new shell. Even then, it is safer to destroy and recreate the widgets. The reason you might not want to do this is performance and the fact that it might not be possible to create the new widget hierarchy with exactly the same attributes as the old. Eclipse uses Control.setParent() for "tear off" controls. So, I would suggest strongly that rather than fighting fundamental design decisions of a library, that they be embraced. This is exactly what SWT did with Windows in exactly this case. The CreateWindowEx() Windows API needs the parent so SWT needs the parent. Hope this helps. First of all, thank you for detailed explanations.
From technical viewpoint I completely agree with you. But from design viewpoint
passing parent into children has some deficiencies. You are probably aware of
them so I will try to be brief.
1. Under such approach children become partly responsible for parent-child
relationship management. In our framework we would have to add to root hierarchy
class some code to add itself to parent. It is not very good.
2. Because we have to pass parent to each child the dependencies between classes
are growing.
3. In practice it makes quite complicated the situation when you have to place
child control to one or another parent depending on child's internal state.
Although I have to admit that such a situation is not very common in practice.
Now about the add() method. It seems quite logical to me to have a convention
that adding control to another parent removes it from its original parent. I do
not think that storing reference to parent in child would be much of a problem
if the whole framework was build on the add() methaphor. Then child controls
would not have to know about its parent at all. Such a mistakes would be as rare
as for ordinary objects.
And after all sometimes you have to reparent controls ("tear off" case). BTW,
how do you do it in Eclipse on Mac OS X?
So, I agree that if you pass parents into children than framework
implementation is simpler. But I think that the most important thing is the
client API even if the implementation is more complicated. In our framework we
initially create controls with invisible parent window. And on add they are
being assigned valid parent. This makes client code cleaner. Just compare:
1. SWT design
Shell shell = new Shell();
new Button(shell);
2. Our framework design
Shell shell = new Shell();
shell.add(new Button());
Not much of a difference but the code is simpler and easier to maintain.
In any case, thank you for great framework! We have already forgot about Swing
slowdowns.
When you add() an object to a collection, does it remove it from another collection it might be in? Is there any case where you would create a control and never add() it to a parent? If not, why not just pass around the parent control and combine the steps? Not sure how this causes any problems. From org.eclipse.swt, "The SWT component is designed to provide efficient, portable access to the user-interface facilities of the operating systems on which it is implemented.". That is number one for us, more important than having and elegant API. So, I strongly urge you to embrace the toolkit rather than fighting it. In the end, your code will be faster and you'll have less to debug and ... thanks for using SWT! *** Bug 89051 has been marked as a duplicate of this bug. *** A gentleman from apple was inquiring about this in bug 56793. Thanks. Send him over! Hi Steve. Should this be re-tagged as a Mac bug? Yup. Created attachment 21955 [details]
patch for Control.java
This patch is my (naive) attempt to get reparenting working on MacOS X by using
the EmbedControl API.
However, my hack is incomplete since I didn't try to get the required menu
reparenting right.
Thanks, Andre. I've asked engineering to take a look at your patch. We'll be in touch. Andre, engineering reports that your use of the EmbedControl API looks fine; the use of that API doesn't look like a "hack" at all. Is there anything else you need? Is anything happening with this bug? Is the suggested patch a start? This fell off a cliff for 3.2. The patch is a good start. Will this be resolved in 3.2 final ? No. We are long past making any code changes for 3.2. And 3.3? I'm interested in this too since the new min/max story works best when Fast View behaviour can be supported (which requires reparenting). Yes this feature is very important for my application too, someone could give us some update about this ? Thanks in advance. Any chance we could see this in 3.3 final ? Any chance we could see this in 3.3 final ? We are unlikely to have time to work on this for R3.3. If someone from the community wants to take a stab at it, we'd be happy to talk to them about it. This functionality is useful for a project that I'm working on, so I'm going to look into implementing support for it. I corresponded with Steve via email, and here's what he had to say: "It's a detailed job. First of all, you need to find the operating system call that reparents the underlying operating system control. Then, you need to write the code that traverses the widget that was reparented and fixes up any fields or operating system state that we have in Java to know about the new parent. This involves fixing popup menus etc. On Windows and GTK, where reparenting is supported, you can see the code pattern we use to do this (a recursive descent with arguments). The Mac code pattern should be similar." Created attachment 57800 [details]
Second iteration of a proposed patch
Okay, here's my first run at a patch. Generally, it looks like the Mac should be pretty easy. Tool tips appear to transfer just fine with their components without any modification. The Webkit-based Browser class used by the Javadoc view in Eclipse doesn't detach gracefully, I'm going to look into it, but any suggestions would be appreciated. Also, at the moment I'm not doing anything to set up the menu bar for the reparented window. Looking at a couple of Apple applications, it seems like the menu bar stays static for pretty much everything within the app (including modal dialogs, etc.), it just greys out unavailable options. Any opinions on what the behavior should be, and if we are transferring menus in some respect, at what level we should be doing it (e.g. reparenting vs. a client thereof)?
>The Webkit-based Browser class used by the Javadoc view in Eclipse >doesn'tdetach gracefully
The Browser webViewHandle is not a child of the Browser handle for some reason I don't remember right now. It is a child of the root control. Look for the code below. You need to reparent it yourself when moving to a different window. Note that there are some callbacks hooked in the Shell that need to be transfer too.
int[] contentView = new int[1];
OS.HIViewFindByID(OS.HIViewGetRoot(window), OS.kHIViewWindowContentID(), contentView);
OS.HIViewAddSubview(contentView[0], webViewHandle);
Great. Thank you very much for your patch. If it is usefull for you, can you please quickly explain how I can apply and test this patch on my computer. I download the patch and then ? I am using Eclipse 3.2.1 for Mac OS X, Intel Mac OS X 10.4.8 with latest java from Apple Keep up the good work. Thanks again Created attachment 57888 [details]
Third iteration of the patch - with Webkit fix
I've updated the patch with code to deal with reparenting the Webkit control and its listeners. I'm still not doing anything menu-wise. That's it in terms of bugs that I've found playing around. Still not sure what to do in terms of menus, though.
Steve asked me to remind him that the latest patch seems to be functional on my end. Could anybody else in the community who is perhaps less swamped, and has a CVS build set up on the Mac test it and corroborate that it works? Also, he asked for the community to nicely make some noise, so...erm, nicely make some noise, I suppose :). From my perspective reparenting menus is less interesting than reparenting higher order structures (Views...) since they're low in embedded state and, in eclipse in particular, can be re-created on demand from internal caches. The problem with the destroy/re-create style of reparenting is in the transfer of the previous control's state information over to the newly created control. This can be fiarly involved for more complex controls such as the Package Explorer (current scroll settings, which sub-trees are open, which -listeners- have to be rehooked...). One of the major advantages of 'true' reparenting is that it removes the onus for this type of code from our clients... Eric, I think you may be misunderstanding what exactly is being added. Carbon has native support for reparenting (although it's apparently called embedding in Carbon-ese), SWT just hasn't been using it, and this is what this patch corrects. Any talk of menus in specific has just been in regards to context menus (which I believe have proved problematic on other platforms, but seem to transfer gracefully under Carbon) or in regards to the question of what to do with the application menu at the top of the screen under OS X, which SWT treats as being associated with a particular shell (much like the menus operate in Windows or GTK), but which most OS X applications treat as being associated with the application directly. Fixed > 20070220. Thanks for the help. Awesome ! Thank you Fred Wulff and everyone for your work on this.. Great! I've applied the patch to my RCP app and it works flawlessly! I think Mac people using multiple monitors will consider this a big improvement. Well done. Is it too late for 3.3? It's in 3.3. The patch (while worthy) had a bunch of problems (internal methods became public breaking portability, missing cases etc.) so we couldn't really use it. The idea was right though. Thanks Fred! Hi, Glad to hear that ! Thanks everyone, but... I am not able to make it work yet. Is it in build I20070220-1330 ? (In reply to comment #36) > Hi, > Glad to hear that ! Thanks everyone, but... I am not able to make it work yet. > Is it in build I20070220-1330 ? No, it is going to be in the next integration build (or a nighty greater than 20070220). I just checked this out in the latest integration build. Awesome! Many thanks for this! |