Bugzilla – Full Text Bug Listing
|
Description
Patrick Chuong
Created attachment 180522 [details]
Patch against 3.6.x.
This is the same patch as we use in our product. It's against 3.6 as we haven't moved to 3.7 yet.
Created attachment 180526 [details]
Patch against head.
In this patch I ported the changes to head and I removed the restriction for the active view.
In our version of the patch the only active provider can be the debug view. Without this restriction, when the user enters a pinned view, then all other views will use the pinned view's context as the active debug context. This is one of the problems that will need to be solved for this patch to go forward.
Another change that may be objectionable is the behavior of IDebugContextService implementation. Currently if a listener registers for a part ID (by calling addDebugSelectionListener(IDebugContextListener listener, String partId), then this listener will be called only if there's a provider registered with that same part ID. In practice this means that a view which uses this listener API would be blank if it's not pinned. I changed the behavior here so that if the view is not pinned (and there is no context provider tied to the given part id), then the listener will be called with updates from the active debug context. IMO this is a logical change and could be considered an obvious bug needing to be fixed, but it could also be viewed as an API change.
Created attachment 180621 [details]
Complete patch for 3.6.x.
I did not review the patch in detail, but noticed that actions are added to open new views. I thought that the platform would just enable multiple views, but not add actions to create/open new view instances? That was going to be up to the extenders? (In reply to comment #4) > I did not review the patch in detail, but noticed that actions are added to > open new views. I thought that the platform would just enable multiple views, > but not add actions to create/open new view instances? That was going to be up > to the extenders? You're correct, the patches are taken straight out of our product and are just a starting point for this bug. Unfortunately, I don't have time right now to follow up on the changes needed to make these patches clean enough to commit, so I hope Patrick can help us with that. (In reply to comment #5) > so I hope Patrick can help us with that. I'll follow up with this patch later this week, I need to take care few issues in our product first. Pawel, I am getting 2 errors when I try to apply the 3.6.x patch against head. Can you elaborate the purpose of IBug14563Marker interface? (In reply to comment #7) > Pawel, I am getting 2 errors when I try to apply the 3.6.x patch against head. That's because it's a patch against 3.6 maintenance stream ;-) I already ported it to head in attachment from comment #2. > Can you elaborate the purpose of IBug14563Marker interface? Our product has a drop-in requirement where we need to be able to drop in our plugins into an unpatched Eclipse installation. We check for this marker interface using reflection to determine whether pin and clone behavior is supported then we disable some features if it's not. Thanks I missed your comment, I'll migrate it to head. Can the marker interface be done differently? Without this interface, you should still be able to use reflection to find out which package DebugContextManager belongs to and do thing differently like the way you check for the marker interface. Are you ok if I remove this interface? We can probablly do something differently if neeeded. Created attachment 181020 [details]
Platform Scope Patch
Pawel/Darin, I tried to tackle the issues that Pawel mentioned in his patch, but I wasn't able to find a clean solution yet.
I have take a different approach, using the scope concept that mentioned in the old bugzilla report. With minimum changes in the platform, I was able to get the view to pin to a set of debug context. There is no side affect or behavor change in term of debug context change event notification.
I'll upload the test plugin that I used to prove the concept of the scope patch. However, the patch is not yet complete, it required more changes to handle debug element get out of scope. I just want to throw it out to see if this is the right thing to do or not.
Created attachment 181021 [details]
Test plugin for scope patch
Created attachment 181132 [details]
Patform DebugModelProvider Patch (Pawel's patch) 3.7
My attempt to clean up Pawel’s patch and address his concerns. I’ll upload the client code separately.
Any debug framework that uses this patch is required to make adjustment to the view model adapter. In the createViewModelProvider method, one will need to adjust how the contextID is compare and create the appropriate view model. I didn’t provide this change with this patch, it should be a simple change. Such as
if (context.getId().startsWith(IDebugUIConstans.ID_EXPRESSION_VIEW)) {
return new ExpressionVM();
}
At the moment I don’t know what else is required to change due to the contextId change in the platform patch. On the other hand, the scope patch does not require such change. I would like feedback regarding which option fit the community’s need and I’ll continue to evolve on one of these solutions.
Created attachment 181133 [details]
Test plugin for DebugModelProvider patch
(In reply to comment #12) > Any debug framework that uses this patch is required to make adjustment to the > view model adapter. ... Hi Patrick, I used the combined presentation context ID in Wind River's patch because it didn't require an API change. And I couldn't change the APIs because it would break our implementation. With the freedom to change APIs we could just add the second ID to the IPresentationContext. This may have more wide spread consequences, but it would be a much cleaner change. (In reply to comment #10) > Created an attachment (id=181020) [details] > Platform Scope Patch Could you refresh our memories as to what the scope did and what where the issues with it. It's been a while. (In reply to comment #14) > With the freedom to change APIs we could just add > the second ID to the IPresentationContext. This may have more wide spread > consequences, but it would be a much cleaner change. Yes, changing the presentation context will requires changes in all debugger that want to support pin&clone. I think this has a signification impact on existing debugger to support this feature. I am hopping minimal changes (or no change) for any debugger to have this feature included, with the exception of the pinned element presentation (color, icon, etc...). If the platform flexible viewer supports JFace ILabelDecorator, than we could protentially decorates the element in the debug view without the needs of model change for each debugger. IDebugContextService also needs to change as well, currently it only takes a partId as an argument for addDebugContextListener and removeDebugContextListener. The current patch provides an id of combined primary id and secondary id, it would be cleaner like what you have suggested for to the IPresentationContext. > (In reply to comment #10) > > Created an attachment (id=181020) [details] [details] > > Platform Scope Patch > Could you refresh our memories as to what the scope did and what where the > issues with it. It's been a while. The scope protype basically filters debug event for listener by calling back to determine whether the source element belongs to the listener or not. I don't recall all the issues for the old scope prototype. The one that I can recall were: - the scope plugin take controls of the default debug context manager using adapter mechanism. - it reimplements the debug context service - not using flexible model, still using IThread, IDebugTarget, IStackFrame - color, presentation, etc.. (not part of the prototype, but were in previous discussions) I tried to simplfied the old scope protype with the new scope patch. Since now that we can modify platform code, I have more freedom to drop many changes in the old scope plugin and make the change directly in the platform code. The new scope patch is incomplete, it required more work. I just want a proof of concept to see how much changes are required in the platform to support scope. (In reply to comment #15) > Yes, changing the presentation context will requires changes in all debugger > that want to support pin&clone. I think this has a signification impact on > existing debugger to support this feature. I don't understand why you think so. To be more specific: I think we could add an IPresentationContext.getSecondaryId(), which would be filled in with the site secondary ID. Existing content providers wouldn't need to change because the primary ID would still be the same. And they would work for the first as well as other instances of views. > IDebugContextService also needs to change as well, currently it only takes a > partId as an argument for addDebugContextListener and > removeDebugContextListener. The current patch provides an id of combined > primary id and secondary id, it would be cleaner like what you have suggested > for to the IPresentationContext. Yes this is true, although this may be a good thing. If we define a new listener method we could explicitly define the behavior for such listeners and avoid the breaking of API behavior that Samantha objected to in bug 145635 comment 13. > I just want a proof of concept to see how much changes are required in the > platform to support scope. OK, I'll wait to look at it till you're ready. Also, that feature should probably go back to bug 145625, let's leave this bug to just focus on enablers for 3rd parties. Unless I'm not understanding it correctly. (In reply to comment #16) > I don't understand why you think so. To be more specific: I think we could add > an IPresentationContext.getSecondaryId(), which would be filled in with the > site secondary ID. Existing content providers wouldn't need to change because > the primary ID would still be the same. And they would work for the first as > well as other instances of views. You are right about the view using the same primary ID. > OK, I'll wait to look at it till you're ready. Also, that feature should > probably go back to bug 145625, let's leave this bug to just focus on enablers > for 3rd parties. Unless I'm not understanding it correctly. I'll continue to work on the model patch (not scope) and introduce the secondary id in IDebugContextService and as well as the IPresentationContext. Pawel, Darin, I want to make the pin client to be transparent to any debugger, such that the client doesn't have to be aware of the pin client to make use of this feature. However, there will be draw back if a debugger doesn't implement some XYZ interface, such as element goes in/out of scope (i.e stackframe). But the basic pin feature should still be usable to such a debugger. So, I am wondering whether it is the right thing to do or not, is to decorate the debug view using the org.eclipse.ui.decorators extension point. thanks, Patrick I have upload a new screenshot of my current working prototype on the main wiki page, you can take a look and provide comments here or on the wiki. thanks, Patrick Created attachment 181671 [details]
Platform Debug Patch for 3.7
This patch covers the main changes required in platform debug. The test plugin can be use to drive the debug views to pin to a set of selected debug elements in the debug view.
There are still some changes that need to be iron out in the test plugin, as far as I can see it doesn't not require changes in the platform for the remaining work items
- store pin states when debug element is out of scope (i.e stackframe)
- refresh pinned view when debug element is out of scope
Created attachment 181674 [details]
Test plugin for Platform Debug Patch
This plugin should works for all views. I have test the platform patch using this plugin for variable, register, expression, disassembly and memory browser. However, I have to make two minor adjustments for the disassembly view and memory browser by adding the secondary id to the addDebugContextLister's parameter.
Example for disassembly view:
IDebugContextService contextService = DebugUITools.getDebugContextManager().getContextService(site.getWorkbenchWindow());
if (contextService instanceof IDebugContextService2) {
((IDebugContextService2)contextService).addDebugContextListener(
fDebugContextListener, site.getId(), site.getSecondaryId());
} else {
contextService.addDebugContextListener(
fDebugContextListener, site.getId());
}
I am wondering if the debug team have time to spare to review this patch and provide feedbacks if there are anything that need to be change. I would be please to hear any feedbacks, and make the necessary changes to have this patch included in the upcoming release. Here are some initial comments: * IDebugContextservice is documented as "@noimplement & @noextend", so new methods can be added in the original interface (i.e. no need for IDebugContextService2). * Can someone describe the need for IDebugContextProvider#canSetActive()? (is it such that a context provider can decline becoming active even when its part has focus? If so, would it be cleaner to just have the context provider un-register when its debug context no longer exists?). * IPresentationContext2 provides only one method, but I'm not sure it's really required, as IPresentationContext has #getPart(). For the case of a view, that will be an IViewPart, which has an IViewSite, which has #getSecondaryId(). * JFace already has a concept of an ILabelDecorator. Would it be possible to leverage the existing interface rather than adding a new extension point (it would be good to avoid having an extension point refer to an internal interface) I have not had time for a detailed review, but this will help start the process. Hi Darin, thank you for taking the time to look at the patch, my comments are embedded. > * IDebugContextservice is documented as "@noimplement & @noextend", so new > methods can be added in the original interface (i.e. no need for > IDebugContextService2). I didn’t realized this interface have @noimplement and @noextend. I have placed all API into the original interface. Fixed in my local copy. > * Can someone describe the need for IDebugContextProvider#canSetActive()? (is > it such that a context provider can decline becoming active even when its part > has focus? If so, would it be cleaner to just have the context provider > un-register when its debug context no longer exists?). That is the idea. I don’t know what other way I can achieve the case where I want a view to listen to debug event from a private context provider, but doesn’t broadcast debug event to other listeners. In my example, I associate a view to listen to debug event from a private context provider, this private context provider is responsible to delegate debug event to the pinned view’s listener. > * IPresentationContext2 provides only one method, but I'm not sure it's really > required, as IPresentationContext has #getPart(). For the case of a view, that You are right, there is a IWorkbenchPart in the presentation context, I added the new interface just for convenience of the client not to have to figure out the secondaryId from the part. I have removed this interface to have a cleaner patch and let the client retrieve the secondaryId themselves. Fixed in my local copy. > will be an IViewPart, which has an IViewSite, which has #getSecondaryId(). > * JFace already has a concept of an ILabelDecorator. Would it be possible to > leverage the existing interface rather than adding a new extension point (it > would be good to avoid having an extension point refer to an internal > interface) > I have not had time for a detailed review, but this will help start the > process. The presentation context needs to be pass to the decorator to figure out the pinned contexts and to match the decorating element. Perhaps I can extend the IBaseLabelProvider and have a similar interface as ILabelDecorator, with the presentation context as an argument to the decorateABC() methods. Created attachment 182535 [details]
Platform Debug Patch v2 for 3.7
Rework per Darin's suggestions.
Created attachment 182536 [details]
Test plugin for Platform Debug Patch v2
New test plugin for v2.
Created attachment 182636 [details]
Platform Debug Patch v2.1 for 3.7
Found a bug in v2 the patch.
Created attachment 183409 [details]
Patch without view presentation changes.
Hi Patrick,
I finally got a good look at your patch. I like the API extensions for part ID handling. And my main feedback is to keep any changes that deal with view presentation as a separate issue.
There are a few changes: tree view label decorator, the ID suffix in the view name, and some others, which I think are more specific to your implementation of multiple view instances than the are a general enabler. Fortunately, I think all of those changes can still be done in your implementation without patching the platform plugin. Although you may have to resort to using reflection to do it.
On the other hand, what I would like to see here is enabling the memory view to be pinned just as the rest of the views. I added this to the patch, though there is one action: GoToAddress that I wasn't able to fix.
Other minor changes:
- I added convenience functions to DebugUITools to make it easier to manage debug context for a part.
- There was an implied link in the VariablesView between the presentation context id and the view's ID. I removed this link as this is not necessarily a valid assumption.
I only had time to test with single view instances so far, but I wanted to bring this up for discussion.
Created attachment 183413 [details] Updated patch without presentation changes and with memory view changes. (In reply to comment #28) > ... I added this to the patch, though > there is one action: GoToAddress that I wasn't able to fix. After parsing through the memory view APIs, I found a way to fix this one too. Pawel, the views are no longer allow to open multiple instances. Created attachment 183423 [details] Updated (again) patch without presentation changes and with memory view changes. (In reply to comment #30) > Pawel, the views are no longer allow to open multiple instances. Oops, I forgot about that. I have tested the patch, it looks good. I think it is all we need to be able to extend the platform to have pin&clone capbility. I guess the only thing remaining is to provides icon and label at the model. I was hopping to use the label decorator to do it generic for all debugger. Is it possible to have this done at DSF level? Or individual DSF extender would rather have it's own implementation of pin&clone? Created attachment 183439 [details] Entry point for extending label provider. (In reply to comment #32) > I guess the only thing remaining is to provides icon and label at the model. I > was hopping to use the label decorator to do it generic for all debugger. Is it > possible to have this done at DSF level? Or individual DSF extender would > rather have it's own implementation of pin&clone? Sorry, I forgot to come back to this problem. This is a difficult problem to address. I agree with Darin, that inventing our own label decorator mechanism is not good, but using the jface decorator API is probably not going to satisfy your needs. Also, I'm not sure we want to have add another official API for customizing element presentation because we already have such a powerful one in the flexible hierarhcy. As an alternative I thought that you could achieve the same effect by extending the internal classes (at your own risk). Specifically I thought you could extend the tree model viewer label provider then set it to the variables view's viewer after you opened it. However, I looked at the label provider and it was missing a method which you could override to achieve this. So this patch would addresses that. I'm fine with including this change as long as it's clear that this is still an internal class and it could change in any release (major, minor, or maintenance). Hi Pawel, I am not trying to decorate the variable views. I intended to decorate the debug view's images and labels, to highlighted the pinned context. I have a picture of the plugin that I used to demonstrate the usage of the decorator at the pin&clone wiki page. I think the picture will help to understand what I would like to achieve. The intend to have the debug view to be decoratable is to enable any provider to extend the debug view's presentation, similar to the other JFace view, i.e C/C++ Project View. Having the debug view decoratable adds the benifit to be able to show pinned context for all debugger without messing with the model. I was trying to come up with a generic solution to enable pin&clone without changing the model (i.e DSF model), as well as debugger that doesn't base off DSF will also benifit from my changes without by dropping in the additional plugin to support pin&clone. The startard label decorator in JFace meets my need to be able to decorate the the debug view. The v2 patch that I attached uses the standard JFace decorator. Pawel, is the patch in a good state to be committed to head? (In reply to comment #34) Thank you Patrick. This is actually the same as I understood it. It just seems to me that it opens too much of an interface for something that does not have a use case in the SDK. Would the small entry point to customize the label provider be enough for you to accomplish what you need? (In reply to comment #35) > Pawel, is the patch in a good state to be committed to head? I apologize for the slow progress, I've been away last week. The latest version I posted would work for me. I'm not sure if Darin has any more comments, also I wonder if Samantha has any opinion on this as she did in the past. I am wondering what the IDebugContextProvider2 interface is used for, noone implements it. It only appears to be used in DebugWindowContextService#partActivated(..). (In reply to comment #37) The pin client uses this interface to delegate debug context event to other client, itself is a context provider, but does not generate debug change event. You can look at the attached plugin for example, the API is renamed in Pawel's latest patch. (In reply to comment #38) > (In reply to comment #37) > The pin client uses this interface to delegate debug context event to other > client, itself is a context provider, but does not generate debug change event. > > You can look at the attached plugin for example, the API is renamed in Pawel's > latest patch. I this that we should at least provide an example implementation of pin and clone view support with the PDA debugger example. It would at least give us a place to point to show how the API is meant to be used. I'll try to come up with an example when I get a chance. In the meantime, you can import the attached plugin to see how it is been used. Created attachment 184302 [details]
PDA example that uses Pawel's patch without presentation changes
This is a very simple patch that demonstrates the PDA pin capability for the Variables view; it doesn't have all the bells and whistles for decorating the views.
You can launch multiple PDA sessions, select the stackframe and hit the pin action. Selecting another PDA session won’t cause the view to update.
The icons are not included in the patch properly, if you need the two icons, I can upload it separately.
I committed the changes including the enabler from comment #33. Patch was previously reviewed. (In reply to comment #41) > Created an attachment (id=184302) [details] > PDA example that uses Pawel's patch without presentation changes I think it would be worth it to permanently add this feature to the PDA example, so that we can at least test this new functionality. Patrick, could you the action icons? Created attachment 184403 [details]
icon files
icon files for the PDA example.
Comment on attachment 182636 [details]
Platform Debug Patch v2.1 for 3.7
IP log changes. The patch is quite large, but patrick's contributions to it were much smaller than the patch itself, therefore I don't believe it needed a separate IP review.
|