Community
Participate
Working Groups
This would be a breaking API change (for the provisional flexible hierarchy API) so I guess it would require some serious deliberation. Currently createModelProxy() just takes a single object: the element. If the element is not at the top of the tree hierarchy, the proxy has to make assumptions about the full path to the element in order to generate a proper model delta (like DebugTargetProxy does, for example). But what's worse is if an element appears in multiple places in the tree, potentially under different sub-trees, then the model proxy has no way of figuring out which location the delta should be generated for. To provide all the information to the model proxy that is needed, I think createModelProxy() should take the viewer input as well as the full path to the element.
To allow for duplicate elements in a view, the path to the element is needed. Viewer input could be provided, but would also be available when the proxy is "installed(..)" in a specific viewer (via Viewer.getInput()). To avoid breaking existing clients I would suggest we create an optional new interface (like IModelProxyFactory2).
Great, I'm glad you agree :-) I will provide a patch for this soon.
I guess I was a little hasty targeting this for M1. I'll see how the VirtualTreeModelViewer (bug 242489) work plays out before I make these changes. This will let me avoid merging, and use the unit tests to verify the changes.
Pawel, are you still planning this change for 3.5?
I don't have any current issues from this bug which I can't work around, so I'm fine putting it off. In the next release cycle (hopefully 4.0 :-)) I would like to revisit the flexible hierarchy interfaces and see if there other changes we should consider before making these APIs final. That would be a better time to come back to this bug as well.
I'm looking into bug 238956 which needs this bug when not a single model is used for the breakpoint containers and for the breakpoints themselves. Is there any proposed patch for this issue or should I provide one? I did locally implement a IModelProxyFactory2 which passes a TreePath instead of an Object and extended the ModelContentProvider to support that. I was also wondering where if there are unit tests in this area, the org.eclipse.debug.tests plugin in cvs head appears to be empty.
Created attachment 146684 [details] Adds IModelProxyFactory2 which is used to install per path model proxies. Proposing patch to support per tree path model proxies for bug 238956 in order to provide proper deltas for breakpoints shown at multiple locations in the (flex) breakpoints view.
Created attachment 146697 [details] example use of IModelProxyFactory2 in CDT/CDI Example usage of IModelProxyFactory2. The main purpose of the patch is to show the use of IModelProxyFactory2 for discussion/tests and it is not intended for now as proposal to be added to CDT. This clearly would need another bug in Bugzilla. The patch is against the head of CDT, it adds in the flex breakpoint view one child entry for each debug target a CDT breakpoint gets installed in. Notes: The patch is functional however it would need a bit of work to include it into CDT. The main shortcomings are that the patch works for the CDI debug model and therefore would need to be adapted to coexist with a DSF based ICBreakpoint extension. As secondary thing is that I limited the patch to display a single breakpoint instance per IDebugTarget only. This limitation is caused by the existing breakpoint model in the CDI layer, in order to support multiple instances per debug target/breakpoint an extension to the CDI layer is necessary (and this patch does not include such an extension. On the good side this means that it works against a non modified gdb/cdi debugger). The patch expects the flexible breakpoints view patch (bug 238956) to be installed as well. Also the last flexible breakpoints view patch does not install proxies of already existing breakpoints when the input changes. This will have to be addressed in the patch to bug 238956 (Note that Patrick is on vacation this week)
Great, thanks for producing this patch already. Adding unit tests will definitely help validate such logic changes. I hope to get to populating the new test plugin a little later this week.
Fixing IModelProxyFactory.createModelProxy() is a good start, but I wonder whether we shouldn't tie this change to a couple additional improvements in the flexible hierarchy framework: 1) A proper fix for race condition between IModelProxy.installed() and IModelProxy.disposed(). This could be accomplished by introducing an IModelProxy2, which would be returned by the new IModelProxyFactory2. 2) An improved version of IElementMementoProvider. I think we could have a more reliable implementation if the memento provider didn't deal directly with mementos, but rather used IPersistableElement. Perhaps the second issue should be dealt with separately...
How could we change IModelProxy2 in order to avoid the race condition? I see that IModelProxy.dispose can currently be called while IModelProxy.installed is being executed (or just about to be executed), is this the issue 1)? I could think of an additional uninstall method which would be called only if installed did succeed, independent of dispose. And what is the issue with the IElementMementoProvider interface? IPersistableElement would have to be implemented by the input directly, or? As far as I know the IElementMementoProvider is used to preserve the selection in the viewers and this is a weaker condition that to be able to persist and reconstruct elements, the purpose of IPersistableElement.
(In reply to comment #11) > How could we change IModelProxy2 in order to avoid the race condition? > I see that IModelProxy.dispose can currently be called while > IModelProxy.installed is being executed (or just about to be executed), is this > the issue 1)? I could think of an additional uninstall method which would be > called only if installed did succeed, independent of dispose. I'll create a patch for it today, so it'll be easier to discuss. But my main problem with the existing IModelProxy.init() is that it's called in a job, which could happen a long time after the proxy is discovered by the viewer. IMO this job really belongs in the proxy itself. Does this warrant a replacement interface... I guess I'd like to find out. > And what is the issue with the IElementMementoProvider interface? > IPersistableElement would have to be implemented by the input directly, or? > As far as I know the IElementMementoProvider is used to preserve the selection > in the viewers and this is a weaker condition that to be able to persist and > reconstruct elements, the purpose of IPersistableElement. I'll file a separate bug and patch for this. Hopefully today if I get to it.
How should the initial state of the content provider be handled by the model proxy? First I thought that the model proxy should only report changes. This fails for the initial state as we depend on the model proxy to explicitly install proxies for sub, elements. Therefore I did add to fire a initial "install proxy for all preexisting breakpoints" model delta when the breakpoint input proxy was installed, probably this is ok with the comment that the install delta's are a bit special as they do not really reflect changes in the model. A similar issue is the timing of the creation of model proxies and the element content provider. How should the model proxy make sure that it provides all the model deltas starting from the initial state, if the initial state is requested asynchronously and independently from the model proxy? As last question is how should we manage the lifetime of the model for the breakpoints view? The case is a bit different for the breakpoints view as the model is created for the view, it is not a model following something external. Right now the model is created when the model proxy gets created, this does basically work but it opens the window for the timing issue as mentioned in comment 12, so I had to add ADDED notifications for all initial breakpoints as well to fix a ordering dependency.
(In reply to comment #13) > How should the initial state of the content provider be handled by the model > proxy? First I thought that the model proxy should only report changes. This > fails for the initial state as we depend on the model proxy to explicitly > install proxies for sub, elements. Therefore I did add to fire a initial > "install proxy for all preexisting breakpoints" model delta when the breakpoint > input proxy was installed, probably this is ok with the comment that the > install delta's are a bit special as they do not really reflect changes in the > model. I think a good corresponding example is the LaunchManagerProxy. > > A similar issue is the timing of the creation of model proxies and the element > content provider. How should the model proxy make sure that it provides all the > model deltas starting from the initial state, if the initial state is requested > asynchronously and independently from the model proxy? Good point. The only time this is a problem is if the content is retrieved from the content provider _before_ the model proxy is installed for the parent element. Also, the model is responsible for requesting that the proxies be installed, so theoretically the model should be able to request that the proxies be installed at the same time or before the content provider is called. However there are two places that delay the installation of the proxy: 1) The install proxy job that this bug is all about. 2) The ModelContentProvider.modelChanged uses a display job. This may be necessary if the listener method is called on non-UI thread, but it's not otherwise. > As last question is how should we manage the lifetime of the model for the > breakpoints view? The case is a bit different for the breakpoints view as the > model is created for the view, it is not a model following something external. > Right now the model is created when the model proxy gets created, this does > basically work but it opens the window for the timing issue as mentioned in > comment 12, so I had to add ADDED notifications for all initial breakpoints as > well to fix a ordering dependency. Why not link it to the lifetime of the view? Linking it to the model proxy installed/disposed would cause it to be created destroyed rather often.
Created attachment 148045 [details] Updated patch. After some experimentation I don't think anymore that it makes sense to combine this change with the race condition fixes for IModelProxy. It would only delay implementing this fix. This patch changes the IModelProxyFactory2.createTreeModelProxy to explicitly specify the viewer input in addition to the path. The previous version of the fix added the viewer input to the path, which is inconsistent with other uses of TreePath. Another change is that I don't have IModelProxyFactory2 extending IModelProxyFactory. I think it only created confusion about which method should be implemented and which interface should be registered with the adapter factory. Also I extended the unit tests to test the new model proxy. I had to introduce the concept of composite models to the test model which took some work, and I found some bugs with the test model in the process. The only remaining question I have is whether we should deprecate IModelProxyFactory. This would mean that we should probably convert the default model proxy factories in Platform to use it. I don't really see any benefit in doing this besides trying to clear up the confusion of which interface to use for the users of this API. Lastly, when doing this work I realized that there is a fundamental limitation on the model proxies which are installed in the middle of a viewer's hierarchy. In order to issue EXPAND and SELECT deltas, they still need to know the indexes and child counts in the parent tree path. I suppose they could retrieve them generically using the elements' IElementContentProvider, but it would be a complicated endeavor at best.
Darin, could you bless this change before I commit it?
Comments: * IModelProxyFactory2 needs its javadoc updated for the 'input' parameter * The tests use Java1.5 code that does not compile (StringBuilder). * I'm going to split the patches between tests and UI plugin... Pawel, did you write all the tests? else the patch is > 250 lines. The changes in UI are about 150 lines, so we do not need a CQ for this (as long as Pawel wrote the tests).
Created attachment 148163 [details] debug.ui patch
Created attachment 148164 [details] debug.tests patch
I see the original patch is < 250 lines. +1 from me with the updates provided.
I committed the fix. Sorry about the confusion from combining the patches. The original patch wasn't very big and I marked it as iplog+ (btw, I'm very grateful for the automatic IP logs right about now).
Marking as verified on Darin's blessing.
Was in vacation for two weeks, so replying a bit late. (In reply to comment #15) >Another change is that I don't have IModelProxyFactory2 extending >IModelProxyFactory. I think it only created confusion about which method >should be implemented and which interface should be registered with the adapter >factory. I see those benefits :-) The main reason what I was extending IModelProxyFactory was to get the support from the platform adabtable implementation to get the most specific proxy, say if the platform provides a IModelProxyFactory2 for a IBreakpoint interface and CDT provides a IModelProxyFactory for ICBreakpoint (just a constructed example), then we should end up using the model proxy for ICBreakpoint. I agree that from the implementation point of view not extending IModelProxyFactory is cleaner. >The only remaining question I have is whether we should deprecate >IModelProxyFactory. This would mean that we should probably convert the >default model proxy factories in Platform to use it. I don't really see any >benefit in doing this besides trying to clear up the confusion of which >interface to use for the users of this API. Changing platform would likely cause the hiding problem mentioned above. > Lastly, when doing this work I realized that there is a fundamental limitation > on the model proxies which are installed in the middle of a viewer's hierarchy. > In order to issue EXPAND and SELECT deltas, they still need to know the > indexes and child counts in the parent tree path. I suppose they could > retrieve them generically using the elements' IElementContentProvider, but it > would be a complicated endeavor at best. I actually do not know enough in which cases a EXPAND and SELECT delta needs/requires the index and the child counts, could the documentation of those flags be updated with which fields are mandatory?
(In reply to comment #23) The adapter contention is a general problem for the flexible hierarchy. Looking at the DSF implementation I also quickly realized that I wouldn't be able to create an implementation just for CDT breakpoints based on the existing proxy interfaces. Since CDT breakpoints are shared between different debuggers, maybe we need to create an extension point in CDT to allow different providers for those breakpoints based on active debug context?
(In reply to comment #24) > (In reply to comment #23) > The adapter contention is a general problem for the flexible hierarchy. > Looking at the DSF implementation I also quickly realized that I wouldn't be > able to create an implementation just for CDT breakpoints based on the existing > proxy interfaces. Since CDT breakpoints are shared between different > debuggers, maybe we need to create an extension point in CDT to allow different > providers for those breakpoints based on active debug context? As alternative to have different providers depending on the debug context, we could just make the children, the breakpoint instances (or installed/planted breakpoints) model specific. So that whenever a CDI debug target (or a DSF debug session) installs a breakpoint that breakpoint would get a model specific child. The breakpoint itself would remain model agnostic (keeping the existing ICBreakpointExtension support of course). Would still need some extension in the base CDT UI, not necessarily a extension point though as the instances only exist when debugging, the models could install them through an interface as well. I did adapt the CDI "test" implementation with the changes make to the APIs, I will attach an updated version here.
Created attachment 148924 [details] updated CDI test implementation
