Community
Participate
Working Groups
There is code all over Eclipse trying to figure out the right parent for a dialog. Should this code simply be using null as the parent and let the framework do it? Or should we have an API that is called to do it? For examples see bug #229668 and bug #229331. For problems see bug #198527 Once this is done, we should encourage people to get rid of the code that's trying to do the right thing (ProgressManagerUtil, ErrorDialog, etc. etc.). There is discussion in bug #229668 of problems with various implementations.
I want to clean this up for 3.5. Tod, I'm really interested in your opinion on: >Should this code simply be using null as the parent and let the >framework do it? Or should we have an API that is called to do it? For example, why did ProgressManagerUtil need to write code vs. pass in a null parent?
I agree with this bug, its a real mess at present. Wrt bug #229668, what we (Tod, Szymon and I) just discussed was that it was wrong for WorkbenchStatusDialogManager to hang onto a shell in the first place. The decision for 3.4 is to ignore the shell field and just call ProgressManagerUtil with no shell. Its believed that ProgressManagerUtil.getModalShellExcluding() doesn't find the topmost shell so a separate bug is to be opened to fix that. All that said, something called 'ProgressManagerUtil' is the wrong place to have that kind of shell management code, and of course its internal. For this bug, we should find more obvious/better place, make it API, and start moving existing code over.
+1 for what Kevin said
Tod, while it is fresh in your mind... Agree that we need to have the correct code in one place. Probably a Dialog static method. But what I still don't understand... Should the dialog framework be using this code when opening a modal dialog and the parent shell is null? ie, would we want clients to call this utility method or would we say, "just pass in a null parent and all the right stuff will happen." It seems like all this code exists because the framework wasn't picking the right parent in the first place...
I would suggest SameShellProvider(null) is the right way to use this.
(In reply to comment #5) > I would suggest SameShellProvider(null) is the right way to use this. > Agree. Clients should call dialog constructor methods with either SameShellProvider(null) or with null as the Shell argument. And then the right thing should happen with respect to selecting the parent. After looking at the code, I also think that the correct code (currently residing in ProgressManagerUtil) should then be moved into the defaultModalParentShell shellProvider initialized in the Window class. The net result should be that most clients can simply pass a null parent or a SameShellProvider(null) to a constructor and the right thing happens. Tod & Kevin - do you agree with this?
Some thoughts: 1. General comment that we must proceed with caution in this area because in the worst case one ends up with incorrectly modal dialogs that can't be dismissed and lock the UI. 2. The magic code in ProgressManagerUtil needs to be in a workbench package somewhere. That's because the shell parenting algorithm makes use of the active workbench. For this reason, we can't do the following > After looking at the code, I also think that the correct code (currently > residing in ProgressManagerUtil) should then be moved into the > defaultModalParentShell shellProvider initialized in the Window class. because Jface is before Workbench in dependencies. 3. I hesitate at having us "just do the right thing" if a shell isn't specified for dialog creation. As above, the algorithm works for us for the way be believe people use workbenches. This may not apply to other RCP apps. Also, this area is tricky and I hesitate at applying this to use cases we're not sure we understand in totality. But maybe I just don't understand the suggestion. In any case I don't think we can do it anyway because of (2) above. Did this answer your questions Susan?
I'm just heading home so don't have time to check the code but wanted to capture this: As I recall, there are basically two things that ProgressManagerUtil does for us, based on these two cases: Case A: I have a shell, want to open a dialog on it. Result: Climb shells, being mindful of modality, if they're disposed, etc. Case B: I just want to open a dialog "on the right thing" Result: Start with the *active* workbench (could be several), get its shell, do case (A). The code for Case A could be in jface.dialogs. The code for Case B has to be in workbench. Case A would always take a shell as arg and I think null would be illegal. Case B would never take a shell. This split could provide the right behaviour for RCP apps who only rely on jface via A, and the ide/worbench policy'ish code via B.
thanks, Kevin, for the input. I'll pick this up in M5.
K I will help
marking M6, focusing on major feature freeze for M5.
I simply ran out of time before API freeze. Removing milestone.
see bug 275083, we are having another "copy the current code to p2" discussion. Last year we had the same issue (bug 231387). We need to fix this early in 3.6, I'd mark this M1 if the milestone was there... Kevin, I'll work with you to nail this down.
*** Bug 274700 has been marked as a duplicate of this bug. ***
in the dup bug Boris suggested: It might be best to provide an implementation of IShellProvider for this, something like: public interface IWorkbench { ... public IShellProvider getDefaultParentProvider(); } (not sure about the method name, and if IWorkbench would be a good place to put this) the implementation would create an anonymous implementation of IShellProvider that forwards to ProgressManagerUtil.getDefaultParent().
Created attachment 159383 [details] patch to ui.workbench This patch implements Boris' most recent suggestion. A method is added to IWorkbench with returns a shell provider appropriate for a modal dialog. The new method is called IWorkbench.getModalDialogShellProvider() The method is stateless (though it does retrieve all workbench windows when there are no modal shells up) so theoretically it could go anywhere. I considered adding it to PlatformUI, but the method looked out of place there. It doesn't belong on IWorkbenchWindow since the method looks at all workbench windows. It also seems to make sense on IWorkbench since this shell provider could be passed to IWorkbench.saveAll(...). I considered also adding a ModalDialogShellProvider class in JFace. This provider would be instantiated with a non-modal shell or null, and its behavior would be defined to return the appropriate modal parent for a shell, or the provided shell if a modal dialog is not open. I was going to move the code from the ProgressManagerUtil.getModalShellExcluding(...) methods into this provider, so that RCP/JFace apps could get the same behavior in a workbench-free way. However, those methods are tricky and the "excluding" behavior is really not that easy to explain or define. It seems rather specific to the jobs dialog. Even if we sorted this out in a way that made sense, we wouldn't want to use this provider by default. So it seemed best to wait until we have a real use case rather than try to anticipate the needs in this area.
(In reply to comment #16) > I considered also adding a ModalDialogShellProvider class in JFace. This > provider would be instantiated with a non-modal shell or null, and its behavior > would be defined to return the appropriate modal parent for a shell, or the > provided shell if a modal dialog is not open. I was going to move the code > from the ProgressManagerUtil.getModalShellExcluding(...) methods into this > provider, so that RCP/JFace apps could get the same behavior in a > workbench-free way. However, those methods are tricky and the "excluding" > behavior is really not that easy to explain or define. It seems rather > specific to the jobs dialog. Even if we sorted this out in a way that made > sense, we wouldn't want to use this provider by default. So it seemed best to > wait until we have a real use case rather than try to anticipate the needs in > this area. After digging around further, I see that the ProgressManagerUtil code originated from the "defaultModalShell" in JFace Window. This shell provider is used when the window to be opened is modal and there is no parent shell specified. One possible improvement would be to check for modal shell children of the specified parent, but it's not clear this would be the right thing to do in all cases. In other words, rather than expose a ModalDialogShellProvider, we might be better off to just "do the right thing" in the defaultModalShell, but this is dangerous. A further complication is that many of the popular dialog subclasses (IconAndMessageDialog, TitleAreaDialog, etc.) were written before the notion of IShellProvider, so they take assume that a shell (not a shell provider) is passed into the constructor. As long as this is the case, then there are likely to be problems in this area.
Boris, do you concur with the name and placement of this method: IWorkbench.getModalDialogShellProvider() Also note that in practice, many clients of this API will have to use PlatformUI.getWorkbench().getModalDialogShellProvider().getShell() This is because message dialogs, wizards, and other title area dialogs require a static Shell on construction, not IShellProvider. The IShellProvider can be used for some dialogs, and API such as IWorkbench.saveAll(...)
Looks like a very good addition to me. We should make that method popular, e.g. by putting it into /org.eclipse.platform.doc.isv/porting/3.6/recommended.html and/or announcing in the N&N.
(In reply to comment #19) > Looks like a very good addition to me. We should make that method popular, e.g. > by putting it into /org.eclipse.platform.doc.isv/porting/3.6/recommended.html > and/or announcing in the N&N. I will write up the porting snippet while verifying this bug (during milestone wee) and ensure something gets in the N&N.
.
Verified on Win7, Build id: I20100309-0100, via source inspection. (p2 is using this new API as well in M6).
UA is also using this API
