Bugzilla – Full Text Bug Listing
| Summary: | [multicore][Pin&Clone] Pin and Clone support | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Patrick Chuong <pchuong> | ||||||||||||||||||||||||||||||||
| Component: | cdt-debug-dsf | Assignee: | Patrick Chuong <pchuong> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | aleherb+eclipse, cdtdoug, dalexiev, marc.khouzam, marcin.swiezawski | ||||||||||||||||||||||||||||||||
| Version: | 7.0 | ||||||||||||||||||||||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||
| Bug Depends on: | 327263 | ||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Patrick Chuong
Created attachment 184664 [details]
Patch without icon overlay
This is a patch without the icon overlay in the Debug view. This patch should have the Register, Variable, Expression, Disassembly, and Memory Browser working. You should be able to pin the view with multiple threads or multiple sessions. Try both DSF-GDB and CDI-GDB, DSF-GDB has better support for pinning. For DSF-GDB you will get the pinned context in the view's description, as well as re-launching the same launch works on the same pinned context.
Created attachment 184665 [details]
Icon files
Icon files for the pin and clone buttons.
(In reply to comment #1) > Created an attachment (id=184664) [details] > Patch without icon overlay > > This is a patch without the icon overlay in the Debug view. This patch should > have the Register, Variable, Expression, Disassembly, and Memory Browser > working. You should be able to pin the view with multiple threads or multiple > sessions. Try both DSF-GDB and CDI-GDB, DSF-GDB has better support for pinning. > For DSF-GDB you will get the pinned context in the view's description, as well > as re-launching the same launch works on the same pinned context. I applied the patch, except for the memory plugin (I didn't have it checked out). I see the new icons, but when I click on Pin and then change the stack frame, the content of the variables view still changes. This is with DSF-GDB. With CDI, things work ok. Also, I got an NPE when I terminated a DSF-GDB launch, but I can't see it anymore. Maybe it was unrelated. (In reply to comment #3) > but when I click on Pin and then change the stack > frame, the content of the variables view still changes. > This is with DSF-GDB. With CDI, things work ok. DSF-GDB does not pin to the stackframe, you can switch between stackframe when the view is pinned. When stackframe is selected, the pin context re-routed to the Thread. This is the behavior that I mentioned to you in email. Would you like the pin context to be a stackframe when the selection is a stackframe? I can make that change and it will behave the same as CDI. I got the NPE again, when simply terminating a DSF-GDB session. I'm doing a clean/re-build to see if it is the cause of the problem. org.eclipse.swt.SWTException: Failed to execute runnable (java.lang.NullPointerException) at org.eclipse.swt.SWT.error(SWT.java:4083) at org.eclipse.swt.SWT.error(SWT.java:3998) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:137) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3527) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3174) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2629) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2593) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2427) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:670) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:663) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:115) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:369) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:619) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:574) at org.eclipse.equinox.launcher.Main.run(Main.java:1407) at org.eclipse.equinox.launcher.Main.main(Main.java:1383) Caused by: java.lang.NullPointerException at org.eclipse.pde.internal.runtime.registry.model.ServiceRegistration.getUsingBundles(ServiceRegistration.java:76) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.getTopLevelElement(RegistryBrowserModelChangeListener.java:63) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.refreshTopLevelElements(RegistryBrowserModelChangeListener.java:99) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.update(RegistryBrowserModelChangeListener.java:129) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener$1.run(RegistryBrowserModelChangeListener.java:26) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) ... 23 more (In reply to comment #5) > I got the NPE again, when simply terminating a DSF-GDB session. I'm doing a > clean/re-build to see if it is the cause of the problem. Even after a clean, I see this all the time. I have the variables view showing, and I do a local DSF-GDB application debug on Linux. As soon as the launch is finished and I'm stopped on main, I press the Terminate button, and I get the NPE The callstack doesn't looks like something that is from the patch. Can you try to catch on nullpointer exception and see what might have cause this exception? (In reply to comment #7) > The callstack doesn't looks like something that is from the patch. Can you try > to catch on nullpointer exception and see what might have cause this exception? I had stopped on the NPE and ended up on the same spot as where the second part of the dump shows. I don't know why that is related to your patch though. But when I revert the patch, I don't see the NPE anymore. I assume you cannot reproduce it? org.eclipse.pde.internal.runtime.registry.model.ServiceRegistration.getUsingBundles(ServiceRegistration.java:76) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.getTopLevelElement(RegistryBrowserModelChangeListener.java:63) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.refreshTopLevelElements(RegistryBrowserModelChangeListener.java:99) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.update(RegistryBrowserModelChangeListener.java:129) at org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener$1.run(RegistryBrowserModelChangeListener.java:26) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) You have to use the DsfServicesTracker#getService inside the executor. !ENTRY org.eclipse.ui 4 0 2010-12-20 10:50:47.874 !MESSAGE Unhandled event loop exception !STACK 0 java.lang.AssertionError at org.eclipse.cdt.dsf.service.DsfServicesTracker.getServiceReference(DsfServicesTracker.java:177) at org.eclipse.cdt.dsf.service.DsfServicesTracker.getService(DsfServicesTracker.java:222) at org.eclipse.cdt.dsf.service.DsfServicesTracker.getService(DsfServicesTracker.java:207) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.getData(GdbPinProvider.java:80) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.getCombinedLabels(GdbPinProvider.java:120) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.pin(GdbPinProvider.java:155) at org.eclipse.cdt.debug.internal.ui.pinclone.DebugContextPinProvider.pin(DebugContextPinProvider.java:146) at org.eclipse.cdt.debug.internal.ui.pinclone.DebugContextPinProvider.<init>(DebugContextPinProvider.java:45) at org.eclipse.cdt.debug.internal.ui.pinclone.DebugEventFilterService.addDebugEventFilter(DebugEventFilterService.java:90) at org.eclipse.cdt.debug.internal.ui.actions.PinDebugContextActionDelegate.run(PinDebugContextActionDelegate.java:62) at org.eclipse.cdt.debug.internal.ui.actions.PinDebugContextActionDelegate.runWithEvent(PinDebugContextActionDelegate.java:54) at org.eclipse.ui.internal.PluginAction.runWithEvent(PluginAction.java:241) at ... (In reply to comment #4) > (In reply to comment #3) > > but when I click on Pin and then change the stack > > frame, the content of the variables view still changes. > > This is with DSF-GDB. With CDI, things work ok. > > DSF-GDB does not pin to the stackframe, you can switch between stackframe when > the view is pinned. When stackframe is selected, the pin context re-routed to > the Thread. This is the behavior that I mentioned to you in email. Would you > like the pin context to be a stackframe when the selection is a stackframe? I > can make that change and it will behave the same as CDI. If I pin a view to a stackframe/thread, and then I select another thread's stack frame, shouldn't the view not change? It changes for me. I can't seem to see the pinned behavior at all... Created attachment 185557 [details]
Patch without icon overlay (fixed dsf service issue)
Marc,
Can you try this patch? Perhaps you didn't see the pin was due to the assertion error that you pointed out?
(In reply to comment #11) > Created an attachment (id=185557) [details] > Patch without icon overlay (fixed dsf service issue) > > Marc, > > Can you try this patch? Perhaps you didn't see the pin was due to the assertion > error that you pointed out? Things are better now. I can see the pinning work pretty well. However, I'm getting different problems with terminating a DSF-GDB launch. If had the NPE I've already reported, but I've also gotton an NPE at GdbPinProvider.java line 91 with session being null. And I've had a deadlock. I think the best thing would be for me to review the patch, which may help with some of these issues, and should be done anyway. Tomorrow is my last day until next year though, so I'm not sure how far I will get. (In reply to comment #12) > I think the best thing would be for me to review the patch, which may help with > some of these issues, and should be done anyway. Tomorrow is my last day until > next year though, so I'm not sure how far I will get. Yes, please do review the patch, it is always good to have another pair of eyes to look over the code. BTW, I don't see NPE when I terminate, perhaps I need to run on linux and use GDB 7.2. Do you have a test application that you can attach to this bug? Perhaps it can help me to track down the NPE. Tomorrow will be my last day at work and will return after the holiday, will continue on this bug after I return next year. Created attachment 186125 [details]
Patch with unpin method
Marc, I have added an unpin method to the IPinProvider interface and changed the action handler to call the unpin method with the toggle button is uncheck. Pin provider than can use the pin and unpin methods to update the debug view icon. I have implemented the pin overlay icon for our debugger using these two API, as for dsf/gdb I can do the same. However this will required a new API in DSF to update the view by generate delta. Let me know if this is something that you are interested and having new API in DSF to update the model is ok, than I can rework the patch to include the pin overlay in the debug view for dsf/gdb.
I'm reviewing the patch now. I have a UI-related question. Do we need to have the title of the views use the secondary Id? I noticed that this is not something that is done normally, for example when opening multiple Console views. When the view is not pinned, then all instances behave the same, so there probably is no need to differentiate them; while if the view is pinned, then the secondary id does not correspond to the pin decorator so it may not help anyway. It's true that the user can at least use the secondary id to remember manually that "Variables 1" is pinned to something in particular, that way it is easier to find using the title. Is that the reason to put the secondary id in the title? Is there a way to decorate the view title? (In reply to comment #15) > Do we need to have the title of the views use the secondary Id? I noticed > that this is not something that is done normally, for example when opening > multiple Console views. I am following Visual Studio and our debugger, the editor has similar decoration for new text file as well. When a view supports multiple instances, having a different title helps to distingush the view of the same instance. For example, you can have a breakpoint action to refresh a view when it is triggered. User can selects a view from the list to refresh, than having Variables <1> and Variables <2> is much better than seeing two Variables in the list of choice. > When the view is not pinned, then all instances behave the same, so there > probably is no need to differentiate them; while if the view is pinned, then > the secondary id does not correspond to the pin decorator so it may not help > anyway. You are right, the current implementation doesn't tie the view instance to the decorator. The secondary id in the title is used for differentiating multiple instances. > It's true that the user can at least use the secondary id to remember manually > that "Variables 1" is pinned to something in particular, that way it is easier > to find using the title. Is that the reason to put the secondary id in the > title? yes. > Is there a way to decorate the view title? I can't find a way to decorate the title, so I ended up using reflection. (In reply to comment #16) > > Is there a way to decorate the view title? > > I can't find a way to decorate the title, so I ended up using reflection. That's what we do in the WindRiver product as well. (In reply to comment #16) > (In reply to comment #15) > > Do we need to have the title of the views use the secondary Id? I noticed > > that this is not something that is done normally, for example when opening > > multiple Console views. > > I am following Visual Studio and our debugger, the editor has similar > decoration for new text file as well. When a view supports multiple instances, > having a different title helps to distingush the view of the same instance. For > example, you can have a breakpoint action to refresh a view when it is > triggered. User can selects a view from the list to refresh, than having > Variables <1> and Variables <2> is much better than seeing two Variables in the > list of choice. Ok then. Sounds better to have the secondary id. (In reply to comment #17) > (In reply to comment #16) > > > Is there a way to decorate the view title? > > > > I can't find a way to decorate the title, so I ended up using reflection. > > That's what we do in the WindRiver product as well. So there is no way to put the nice little colored decorator in the title icon of the view? I'm still digesting things, but here are some preliminary comments. Things look good to me up to now. 1- Please turn on API tooling: http://wiki.eclipse.org/CDT/policy#Using_API_Tooling, then do a full clean and a new build. You'll need to change the 8.0 versions to 7.1 (because that plugin does not require a major version increment). GdbAdapterFactory 2- Need to call session.unregisterModelAdapter() for the two new pin providers. Do like for IStepOverHandler.class/fStepOverCommand PinCloneUtils 3- setPartTitle() could call decodePinnablePartSecondaryId() instead of doing secondaryId.replaceAll(PIN_CLONE_VIEW_TAG, ""); 4- using reflection in setPartTitle() is very much a hack. But I couldn't find a better way. org.eclipse.cdt.debug.ui/plugin.properties 5- copyright change is missing bug number org.eclipse.cdt.debug.ui/plugin.xml 6- can we use org.eclipse.ui.commands, org.eclipse.ui.menus and org.eclipse.ui.handlers for the new two buttons? Note that I don't fully understand the limitation of viewActions but that I've read multiple times that the commands are much better. You may want to confirm that my request is worth doing with Pawel who know this better than me. Something like this instead <extension point="org.eclipse.ui.commands"> <category description="%PinCloneCategory.description" id="org.eclipse.cdt.debug.ui.category.pinclone" name="%PinCloneCategory.name"> </category> <command id="org.eclipse.cdt.debug.ui.command.openNewView" categoryId="org.eclipse.cdt.debug.ui.category.pinclone" description="%OpenNewView.description" name="%OpenNewView.name"> </command> <command id="org.eclipse.cdt.debug.ui.command.pinView" categoryId="org.eclipse.cdt.debug.ui.category.pinclone" description="%PinView.description" name="%PinView.name"> </command> </extension> <extension point="org.eclipse.ui.handlers"> <handler class="org.eclipse.cdt.debug.internal.ui.actions.OpenNewViewActionDelegate" commandId="org.eclipse.cdt.debug.ui.command.openNewView"> </handler> <handler class="org.eclipse.cdt.debug.internal.ui.actions.PinDebugContextActionDelegate" commandId="org.eclipse.cdt.debug.ui.command.pinView"> </handler> </extension> <extension point="org.eclipse.ui.menus"> <menuContribution locationURI="toolbar:org.eclipse.debug.ui.VariableView?after=variableGroup"> <command commandId="org.eclipse.cdt.debug.ui.command.openNewView" icon="icons/elcl16/open_new.gif" label="%OpenNewView.name" style="push" tooltip="%OpenNewView.description"> </command> </menuContribution> <menuContribution locationURI="toolbar:org.eclipse.debug.ui.VariableView?after=variableGroup"> <command commandId="org.eclipse.cdt.debug.ui.command.pinView" icon="icons/elcl16/pin.gif" label="%PinView.name" style="toggle" tooltip="%PinView.description"> </command> </menuContribution> </extension> Note that OpenNewViewActionDelegate and PinDebugContextActionDelegate should be made handlers and probably should be moved to a commands directory. 7- Shouldn't the adding of the two new buttons for the Disassembly view and Memory browser be done in their own plugins? IPinProvider 8- Typos in the javadoc for the class PinElementHandle 9- Typos in the javadoc for the class 10- equals() should instead do: return ((PinElementHandle) obj).fData.equals(fData); to be safe, we should probably check the case where ((PinElementHandle) obj).fData == null DebugContextPinProvider 11- in isPinnable() the logic does not seem quite right. For example, it allows to pin to the GdbLaunch. The logic should consider cases where some of the parts of the selection are not IAdaptable or are not IPinProvider, etc. Basically adding 'else return false' to all the cases where the if fails. But I think this breaks how CDI does things, although the CDI approach was not clear to me. I have to think about how you did things for CDI. 12- When first launching eclipse, the pin button is enabled, although there is no debug context. I think we should start off disabled (In reply to comment #18) > So there is no way to put the nice little colored decorator in the title icon > of the view? It is possible, use reflection. What myself and Pawel refer to is that there is no extension point to decorate the view's title. We have to resource to reflection instead. Put this piece of code in PinCloneUtils.setPartTitle to experiment with the image. Method method2 = WorkbenchPart.class.getDeclaredMethod("setTitleImage", Image.class); //$NON-NLS-1$ if (method2 != null) { method2.setAccessible(true); method2.invoke(part, DebugPluginImages.getImage (IDebugUIConstants.IMG_OBJS_VARIABLE)); } I'll update the patch with the comments in your last message. I am hoping we can get this patch in before the next CDT-M build and file new bugs for any additional changes. (In reply to comment #20) > (In reply to comment #18) > > So there is no way to put the nice little colored decorator in the title icon > > of the view? > > It is possible, use reflection. What myself and Pawel refer to is that there is > no extension point to decorate the view's title. We have to resource to > reflection instead. Ok, good then. But I guess using the secondaryId is still important in the title for cases like "User can selects a view from the list". I originally thought a decorator in the title would replace the secondaryId, but I see that it would limit things for other situations, like lists. I think I understand why things work for CDI even though there is no code for it. From what I see in DebugContextPinProvider isPinnable() and pin(), you allow that if we don't have an IPinProvider, we can still pin. I find that strange. The javadoc of IPinProvider starts with "Debug element that wants to enable pin capability should be adaptable to this interface." But I don't think you actually enforce this. Wouldn't that make any debugger automatically support pin&clone? Event the JDT one? Is that what we want? Here are a few other minor questions and comments. I just have GdbPinProvider left to review. ViewIDCounterManager: 1- if ( !(part instanceof IViewPart) && !PinCloneUtils.isPinnablePart((IViewPart) part)) should be an || instead of an && 2- shouldn't storeCounterIds() be called from saving() instead of prepareToSave()? 3- Note sure if we are handling this right or not: if I have "Variables" and "Variables <1>" and I close "Variables" and then clone a variables view, I will get "Variables <2>" instead of "Variables". I can only get that view title back by using "Show view" in the Window menu. 4- PinCloneUtils.isPinnablePart() does not consider the first view (e.g., "Variables") as a pinnablePart, although it is, which confused me. Maybe the method should be called isClonedPart()? If so, then encodePinnablePartSecondaryId() should be encodeClonedPartSecondaryId(). Same for decodePinnablePartSecondaryId() PinDebugContextActionDelegate 5- what is getDebugContextListener() meant to be used for and how come it does not return 'this'? 6- in debugContextChanged(), how about checking that fAction.isEnabled() != pinnable to know if we should change the enabled state? 7- in run(), I think when the action is being unselected (the else case), we should check if the action should be disabled based on the currentActiveDebugContext (the code from debugContextChanged()). Or is this already being done with a fake debugContextChanged event? DebugContextPinProvider 8- why is isPinnable() part of this class? Shouldn't it in PinCloneUtils? 9- for consitency (like for ViewIDCounterManager), can you use a getInstance() method and make INSTANCE private? 10- in isPinnedTo(), we should first check if we have a valid pinProvider outside the while loop. No point in looping if we don't have one. 11- I'm wondering if it is safe that getActiveContext() will always return the context to which we pinned, instead of the actual active context? Say we pin to a thread and then select a frame, shouldn't the active context be the frame? But I believe we will be returning the thread all the time, no? DebugEventFilterService 12- in removeDebugEventFilter() you call contextProvider.delegateEvent() before calling contextService.removeDebugContextProvider(). Doesn't this mean that the part/view still has the DebugContextPinProvider as its provider when it receives the DebugContextChange call? Isn't there a risk that it will call getActiveContext() from that provider and it will not match the active context that we just sent in the event? I was thinking we should first call contextService.removeDebugContextProvider() and then contextProvider.delegateEvent(), however, I'm not sure how to find the ActiveProvider to pass it to new DebugContextEvent()... MemoryBrowser 13- in dispose() do we have to change the removeDebugContextListener() call to match the change for our new call to addDebugContextListener()? Created attachment 186404 [details] Rework to address Marc's comment #19 (In reply to comment #19) Marc, I have updated the patch with changes regarding your comments. I will take a look at your newer comments and will rework the patch more. I don't want to lost track of the changes aginst the comments that you made and create a newer patch. > 1- Please turn on API tooling: done. > GdbAdapterFactory > 2- Need to call session.unregisterModelAdapter() for the two new pin providers. > Do like for IStepOverHandler.class/fStepOverCommand Thanks, you have found a bug. > PinCloneUtils > 3- setPartTitle() could call decodePinnablePartSecondaryId() instead of doing > secondaryId.replaceAll(PIN_CLONE_VIEW_TAG, ""); done. > 4- using reflection in setPartTitle() is very much a hack. But I couldn't > find a better way. I can't find any other ways to change the view title. > org.eclipse.cdt.debug.ui/plugin.properties > 5- copyright change is missing bug number fixed. > org.eclipse.cdt.debug.ui/plugin.xml > 6- can we use org.eclipse.ui.commands, org.eclipse.ui.menus... The problem with the command framework is causing toggle command not to work per command handler. There is on command handler for all view instances, so when you toggle one command, it also affects the other view instance as well. Pawel mentioned in the past he tried to play around with instantiating a handler per view, but gave up on it. Keep in mind that I don't have access to the toolbar action from code, I am contributing the action through plugin.xml file. > 7- Shouldn't the adding of the two new buttons for the Disassembly view and > Memory browser be done in their own plugins? I didn't want to duplicate the changes - code, plugin.properties, icon images in these two plugins. I made the changes any way. > IPinProvider > 8- Typos in the javadoc for the class fixed. > PinElementHandle > 9- Typos in the javadoc for the class fixed. > 10- equals() should instead do: > return ((PinElementHandle) obj).fData.equals(fData); > to be safe, we should probably check the case where ((PinElementHandle) > obj).fData == null fixed > DebugContextPinProvider > 11- in isPinnable() the logic does not seem quite right. For example... I allows pin to anything. For debugger that wants to overrides the standard behavior, than each debug element can be adapt to IPinProvider. > 12- When first launching eclipse, the pin button is enabled, although there is > no debug context. I think we should start off disabled fixed. (In reply to comment #23) > > Marc, I have updated the patch with changes regarding your comments. I will > take a look at your newer comments and will rework the patch more. I don't want > to lost track of the changes aginst the comments that you made and create a > newer patch. I think that is a good way to work. It is easier to review when the changes are clearly mapped to a set of comments. Thanks for the fixes. > > 1- Please turn on API tooling: > done. - in IPinProvider there is an extra @since 8.0 above IPinHandleLabelProvider > > PinCloneUtils > > 3- setPartTitle() could call decodePinnablePartSecondaryId() instead of doing > > secondaryId.replaceAll(PIN_CLONE_VIEW_TAG, ""); > done. The new line secondaryId = decodePinnablePartSecondaryId(PIN_CLONE_VIEW_TAG); should be secondaryId = decodePinnablePartSecondaryId(secondaryId); > > org.eclipse.cdt.debug.ui/plugin.xml > > 6- can we use org.eclipse.ui.commands, org.eclipse.ui.menus... > The problem with the command framework is causing toggle command not to work > per > command handler. There is on command handler for all view instances, so when > you > toggle one command, it also affects the other view instance as well. I hadn't thought of it. Ok then, let's forget it. > > 7- Shouldn't the adding of the two new buttons for the Disassembly view and > > Memory browser be done in their own plugins? > I didn't want to duplicate the changes - code, plugin.properties, icon images > in these two plugins. I made the changes any way. I think that is more appropriate. Thanks. > > 10- equals() should instead do: > > return ((PinElementHandle) obj).fData.equals(fData); > > to be safe, we should probably check the case where ((PinElementHandle) > > obj).fData == null > fixed - what do you want the equals() to really do? I assumed it should basically ignore the label field. If that is the case, I think fData == null should be checked explicitly: public boolean equals(Object obj) { if (obj instanceof PinElementHandle) { if (fData == null) { return (PinElementHandle) obj).fData == null; } else { return fData.equals(((PinElementHandle) obj).fData); } } return false; } > > DebugContextPinProvider > > 11- in isPinnable() the logic does not seem quite right. For example... > I allows pin to anything. For debugger that wants to overrides the standard > behavior, than each debug element can be adapt to IPinProvider. Let's talk about this when you look at the comments of comment #22. Created attachment 186442 [details] Rework to address Marc's comment #22 (In reply to comment #22) There are some issues when I disable the toggle action during startup, some view instances doesn't instantiate the action delegate. I am not sure how to address this issue. Any thought? > Wouldn't that make any debugger automatically > support pin&clone? Event the JDT one? Is that what we want? Yes, I allow pin to any debugger. I also not sure whether this is the right thing to do. I was planning to have this feature at the platform level, but now that it is a CDT feature,perhaps it make more sense to check element is an instanceof ICDebugElement if the element in question is not adaptable to IPinProvider? see patch. > ViewIDCounterManager: > 1- if ( !(part instanceof IViewPart) && > !PinCloneUtils.isPinnablePart((IViewPart) part)) > should be an || instead of an && Fixed. > 2- shouldn't storeCounterIds() be called from saving() instead of > prepareToSave()? Fixed. > 3- Note sure if we are handling this right or not: if I have "Variables" ... Fixed. > 4- PinCloneUtils.isPinnablePart() does not consider the first view (e.g., > "Variables") as a pinnablePart, although it is, which confused me. Maybe the > method should be called isClonedPart()? ... Fixed - renamed methods. > PinDebugContextActionDelegate > 5- what is getDebugContextListener() meant to be used for and how come it does > not return 'this'? No use, must be left over from my work in progress. removed. > 6- in debugContextChanged(), how about checking that fAction.isEnabled() != > pinnable to know if we should change the enabled state? Good point. Fixed. > 7- in run(), I think when the action is being unselected (the else case), we > should check if the action should be disabled based on the > currentActiveDebugContext (the code from debugContextChanged()). Or is this > already being done with a fake debugContextChanged event? Yes, no need to check in the else statement, the fake debugContextChanged event does the job. > DebugContextPinProvider > 8- why is isPinnable() part of this class? Shouldn't it in PinCloneUtils? Makes sense to be in the utility class. Moved. > 9- for consitency (like for ViewIDCounterManager), can you use a getInstance() > method and make INSTANCE private? You mean DebugEventFilterService, right? Fixed. > 10- in isPinnedTo(), we should first check if we have a valid pinProvider > outside the while loop. No point in looping if we don't have one. I am not sure I know what you mean, pinProvider is per each debug context you want to pin to. > 11- I'm wondering if it is safe that getActiveContext() will always return the > context to which we pinned, instead of the actual active context? Say we pin > to a thread and then select a frame, shouldn't the active context be the > frame? But I believe we will be returning the thread all the time, no? If I understand correct, the active context is for the context provider. The debug view has it's own context provider and active context, so it should be safe. > DebugEventFilterService > 12- in removeDebugEventFilter() you call contextProvider.delegateEvent() ... You can't get the activeProvider, it is private to DebugWindowcontextService. Even if you can, there is a check (Platform Pin&Clone Patch) to see if the provider is a window context provider. In this case, it is set to false. > MemoryBrowser > 13- in dispose() do we have to change the removeDebugContextListener() call to > match the change for our new call to addDebugContextListener()? I fixed both Disassembly and Memory Browser to use the new helper in DebugUITools. (In reply to comment #24) > - in IPinProvider there is an extra @since 8.0 above IPinHandleLabelProvider I have removed already in my patch for comment #22 > The new line > secondaryId = decodePinnablePartSecondaryId(PIN_CLONE_VIEW_TAG); > should be > secondaryId = decodePinnablePartSecondaryId(secondaryId); Found this issue when I worked on the patch for comment #22, fixed. > - what do you want the equals() to really do? I assumed it should basically > ignore the label field. If that is the case, I think fData == null should be > checked explicitly: > public boolean equals(Object obj) { > if (obj instanceof PinElementHandle) { > if (fData == null) { > return (PinElementHandle) obj).fData == null; > } else { > return fData.equals(((PinElementHandle) obj).fData); > } > } > return false; > } You are right, it should be implemented the way you pointed out. Will create a new patch for this change shortly. > > > DebugContextPinProvider > > > 11- in isPinnable() the logic does not seem quite right. For example... > > I allows pin to anything. For debugger that wants to overrides the standard > > behavior, than each debug element can be adapt to IPinProvider. > Let's talk about this when you look at the comments of comment #22. I have made changes to isPinnable to check for adaptable to IPinProvider or instanceof ICDebugElement. Fix is in patch against comment #22. Created attachment 186443 [details] Rework to address Marc's comment #24 (update equals method) Patch regarding comment #24. First, sorry for keeping coming back with more comments, I tend to be picky once I get into a review. I hope I'm not asking for too many things. Just a question for future patches, do you prefer to use iterators over the for contruct? something like Set<String> a;for (String b : a). I personally find it easier to read than an iterator. OpenNewViewActionDelegate - I think the comment // if there is no view without secondary id, than get the next avaliable id. should be // if there is a view without a secondary id already, than get the next available secondary id. - when you set assignSecondaryId = true; might as well break; out of the loop > > 10- in isPinnedTo(), we should first check if we have a valid pinProvider > > outside the while loop. No point in looping if we don't have one. > I am not sure I know what you mean, pinProvider is per each debug context you > want to pin to. But in this method, the debugContext is a single entry so I thought it would be more efficient to first check if it is adaptable to IPinProvider. > > 11- I'm wondering if it is safe that getActiveContext() will always return the > > context to which we pinned, instead of the actual active context? Say we pin > > to a thread and then select a frame, shouldn't the active context be the > > frame? But I believe we will be returning the thread all the time, no? > If I understand correct, the active context is for the context provider. The > debug view has it's own context provider and active context, so it should be > safe. I don't fully understand how it works in the platform, so I could be wrong about this. Normally a view gets it's active context from the activeContext provider which is the debug view. When we pin, we replace the contextProvider with an instance of DebugContextPinProvider right? This context provider is now responsible for telling the pinned view what context is currently active. But the active context is not necessarily the pinned context, it could be a context that is a child of the pinned context. With DebugEventFilter you get events that say that a new context is active in the debug view, and based on the isPinnedTo() method, you decide if the event should be forwarded to the cloned view. This event makes a new context the active context for the view; that is how the view decides to update its content. Should the same logic apply for getActiveContext()? Meaning that if the DebugEventFilter decides to forward and activation context event, that would also mean that the currently active context for that view is the new context? Note that I have not seen any bugs that could show that this is a problem, so I cannot know for sure... > DebugEventFilterService > 12- in removeDebugEventFilter() you call contextProvider.delegateEvent() I'm still now clear about this one, but it is the same issue as the point above about getActiveContext() so let's see what happens there first. - PinCloneUtils#isPinnable() javadoc for the return value says "@return true if all elements are pinnable, otherwise false" but the method returns true as soon as one context is pinnable. I think the javadoc is right and the method wrong, no? > There are some issues when I disable the toggle action during startup, > some view instances doesn't instantiate the action delegate. I am not > sure how to address this issue. Any thought? I saw this too. Not sure what is going on. - Multiple context pinning is not working well. For example, with a multi- selection, I can pin to a GdbLaunch, which I normally cannot. I can also pin to a JDT context if I also select a CDT context. - PinDebugContextActionDelegate#init(), I'm worried that it could happen that fAction may be checked when we disable it (maybe the checked state can be restored when starting up eclipse?). This would cause it to never become enabled again. I actually saw this once. Maybe change the line fAction.setEnabled(pinnable); with if (fAction != null && !fAction.isChecked) fAction.setEnabled(pinnable); ViewIDCounterManager - why do we store the secondary ids in the preferences? Can't we figure them out in the init() method at the same time as we call setPartTitle()? (I hadn't thought of this before) - Do we want to allow to pin to a launch? I'm not sure. How do you see that case? If we do, then the restriction in isPinnable() broke that. If we do not, then I don't think we need to deal with pinLabels for the launch and we can remove GDBLaunchPinHandleLabelProvider GdbPinProvider - in IPinProvider, what are all the IPresentationContext meant to be used for? They are not used in GdbPinProvider so I don't see why they are needed - Can you make getExecutionDmc() and getProcessDmc() take and IDMContext as a parameter? They are always called with one anyway. (In reply to comment #8) I'm still getting the below NPE. I figured out that the cause of it is the line IViewPart part = viewRef.getView(true); from ViewIDCounterManager#init(). changing to IViewPart part = viewRef.getView(false); seems to fix the problem but then the views don't seem to always get re-titled properly. What I did was: 1- change the code to use 'false' 2- run eclipse and open multiple variable views 3- minimize a couple of variable views 4- exit eclipse (file -> exit) 5- run eclipse again and restore the views manually. One of the view (the one that was in focus after the restore) was not properly titled with the secondary id. Any ideas? org.eclipse.pde.internal.runtime.registry.model.ServiceRegistration.getUsingBundles(ServiceRegistration.java:76) > at > org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.getTopLevelElement(RegistryBrowserModelChangeListener.java:63) > at > org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.refreshTopLevelElements(RegistryBrowserModelChangeListener.java:99) > at > org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener.update(RegistryBrowserModelChangeListener.java:129) > at > org.eclipse.pde.internal.runtime.registry.RegistryBrowserModelChangeListener$1.run(RegistryBrowserModelChangeListener.java:26) > at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) > at > org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) Created attachment 186682 [details] Rework to address Marc's comment #28 (In reply to comment #28) I drop support for CDI for now, short circuit CDebugElement in PinCloneUtils. CDI implementation isn't well handle, I'll defer it to later and focus on DSF-GDB for now, will eventually make CDI have the same feature set as DSF-GDB. We can discusss this in the next mutli-core meeting to see if there is any urgent need to have CDI support. > First, sorry for keeping coming back with more comments, I tend to be picky > once I get into a review. I hope I'm not asking for too many things. Not a problem, it means that the patch will get better than the previous iteration. I am thankful that you take the times to review the patches and provides all these valuable comments. > Just a question for future patches, do you prefer to use iterators over the > for contruct? something like Set<String> a;for (String b : a). ... I do perfer to use this as well. If you find this while you review the patch, please let me know and I'll correct it. I fixed the iterator in many places in this patch. > OpenNewViewActionDelegate > - I think the comment ... Fixed. > But in this method, the debugContext is a single entry so I thought it would > be more efficient to first check if it is adaptable to IPinProvider. I see what you mean, I was looking into the wrong method, PinCloneUtils#isPinnable(). Duh! And Fixed. > Note that I have not seen any bugs that could show that this is a problem, so > I cannot know for sure... Now I see what you mean, I believe you are right. DebugContextPinProvider.fActiveContext should be updated, I have made the change to update this member in the delegateEvent. Let me know if this is correct or not. > > DebugEventFilterService > > 12- in removeDebugEventFilter() you call contextProvider.delegateEvent() > I'm still now clear about this one, but it is the same issue as the point > above about getActiveContext() so let's see what happens there first. Do we still need to address this concern? > - PinCloneUtils#isPinnable() javadoc for the return value says > "@return true if all elements are pinnable, otherwise false" > but the method returns true as soon as one context is pinnable. > I think the javadoc is right and the method wrong, no? Fixed the implementation. > > There are some issues when I disable the toggle action during startup, > > some view instances doesn't instantiate the action delegate. I am not > > sure how to address this issue. Any thought? > I saw this too. Not sure what is going on. I'll add this to my TODO list for investigation. So far, this is the first issue that I don't have an answer for you yet. If I missed any any unanswered comment/issue, let me know and I'll track it. > - Multiple context pinning is not working well. For example, with a multi- > selection, I can pin to a GdbLaunch, which I normally cannot. I can also > pin to a JDT context if I also select a CDT context. Fixed, I messed this up in previous patch. Please give this patch a try. > - PinDebugContextActionDelegate#init(), I'm worried that it could happen > that fAction may be checked when we disable it (maybe the checked state > can be restored when starting up eclipse?). ... Yeah, I just saw this happend couple minutes before I got to this comment. Thanks for the fix. > - why do we store the secondary ids in the preferences? Can't we figure > them out in the init() method at the same time as we call setPartTitle()? > (I hadn't thought of this before) It is for the next available ID per view type, otherwise, I have to iterate through all open view and find out what id is been used. Is there another way of doing this? > - Do we want to allow to pin to a launch? ... Pin to the launch doesn't make much sense, I removed the provider. > - in IPinProvider, what are all the IPresentationContext meant to be used > for? They are not used in GdbPinProvider so I don't see why they are > needed For other provider that cares about which instance of the view it is pinned to. > - Can you make getExecutionDmc() and getProcessDmc() take and IDMContext > as a parameter? They are always called with one anyway. Sure, my bad. I was trying different ways to get the pin context, but forgot to make the change. Created attachment 186688 [details] Rework to address Marc's comment #29 (minor update) (In reply to comment #29) Isn't this a bug in ServiceRegistration? Anyway, I do what you did and use "false" for getView and also handles partOpened to update the title. Created attachment 186756 [details] Patch with icon decorator in the debug view. I extended the patch against comment #29 with icon decorator for the thread node, a new event interface is added to IMIExecutionDMContext. I am not total sure this is the right thing to do, can you please provide feedback? I haven't done extensive testing on this change, but so far it looks good. One issue that I see is when you launch the same session more than once at the same time, the pin icon show up in one of the session and not the others. This is a limitation of one debug context per pin handle, however this can be rework, but I don't think user will launch the same session more than once at any given moment. Created attachment 186757 [details]
Icon files with overlay decorator
(In reply to comment #31) > Created attachment 186688 [details] > Rework to address Marc's comment #29 (minor update) > > (In reply to comment #29) > Isn't this a bug in ServiceRegistration? Maybe. Does sound abnormal. > Anyway, I do what you did and use "false" for getView and also handles > partOpened to update the title. Nice! No more NPE. But with this solution I noticed that views that start minimized will show a tooltip (when you hover over the icon of the minimized view) that does not show the updated name. Obviously since we only rename the title once the view is opened. But let's not delay this patch for this. Maybe open a bug for this to explain that it is a workaround for the ServiceRegistration potential issue. > (In reply to comment #30) > I drop support for CDI for now... I agree. Let's get DSF-GDB to work properly and if anywants wants it for CDI, it will be easy for them to immitate the DSF-GDB solution. > > First, sorry for keeping coming back with more comments, I tend to be picky > > once I get into a review. I hope I'm not asking for too many things. > Not a problem, it means that the patch will get better than the previous > iteration. I am thankful that you take the times to review the patches and > provides all these valuable comments. My pleasure, this is a great feature. > > Just a question for future patches, do you prefer to use iterators over the > > for contruct? something like Set<String> a;for (String b : a). ... > I do perfer to use this as well. If you find this while you review the patch, > please let me know and I'll correct it. I fixed the iterator in many places in > this patch. Looks good everywhere. > > OpenNewViewActionDelegate > > - I think the comment ... > Fixed. Sorry, still some typos (some of which I didn't even see before). Should be // if there is a view without a secondary id, then get the next available id. > > But in this method, the debugContext is a single entry so I thought it would > > be more efficient to first check if it is adaptable to IPinProvider. > I see what you mean, I was looking into the wrong method, > PinCloneUtils#isPinnable(). Duh! And Fixed. You still need to check for if (pinProvider == null) return false; to avoid going through the loop for nothing. And then you can remove the check for != null inside the loop. > > > There are some issues when I disable the toggle action during startup, > > > some view instances doesn't instantiate the action delegate. I am not > > > sure how to address this issue. Any thought? > > I saw this too. Not sure what is going on. > I'll add this to my TODO list for investigation. So far, this is the first > issue that I don't have an answer for you yet. If I missed any any unanswered > comment/issue, let me know and I'll track it. I don't see other things left, except the new one I mentioned at the beginning of this mail. Will you open bugs to track those two? > > - why do we store the secondary ids in the preferences? Can't we figure > > them out in the init() method at the same time as we call setPartTitle()? > > (I hadn't thought of this before) > It is for the next available ID per view type, otherwise, I have to iterate > through all open view and find out what id is been used. Is there another way > of doing this? You already iterate over all views in the init() method. So I was originally thinking of addind the following: IViewPart part = viewRef.getView(false); if (part != null && PinCloneUtils.isClonedPart(part)) { PinCloneUtils.setPartTitle(part); // Store the ids that are already being used Set<Integer> secondaryIdSet = viewIdToNextCounterMap.get(viewRef.getId()); if (secondaryIdSet == null) { secondaryIdSet = new HashSet<Integer>(); viewIdToNextCounterMap.put(viewRef.getId(), secondaryIdSet); } String secondaryId = part.getViewSite().getSecondaryId(); secondaryId = PinCloneUtils.decodeClonedPartSecondaryId(secondaryId); secondaryIdSet.add(Integer.valueOf(secondaryId)); } I think this would work ok and would avoid storing and retrieving from a property. The problem is that now that the call to viewRef.getView uses 'false' then we would only find the secondaryIds for view that don't need to be restored, which is not good enough. I then realized you can get the secondary id from the viewRef, and tried this which seemed to work. What do you think of the following: String secondaryId = viewRef.getSecondaryId(); if (secondaryId != null && secondaryId.startsWith(PinCloneUtils.PIN_CLONE_VIEW_TAG)) { Set<Integer> secondaryIdSet = viewIdToNextCounterMap.get(viewRef.getId()); if (secondaryIdSet == null) { secondaryIdSet = new HashSet<Integer>(); viewIdToNextCounterMap.put(viewRef.getId(), secondaryIdSet); } secondaryId = PinCloneUtils.decodeClonedPartSecondaryId(secondaryId); secondaryIdSet.add(Integer.valueOf(secondaryId)); } IViewPart part = viewRef.getView(false); if (part != null && PinCloneUtils.isClonedPart(part)) { PinCloneUtils.setPartTitle(part); } The second line really should use PinCloneUtils.isClonedPart() but that method requires an IViewPart. But I think we can change the method to take an IViewRef, or maybe have a second method isClonedPart() with an IViewRef. what do you think? > > - Do we want to allow to pin to a launch? ... > Pin to the launch doesn't make much sense, I removed the provider. Can you remove the two remaining references to IPinHandleLabelProvider in GdbAdapterFactory? Also, I believe you can remove the change in plugin.xml of dsf.gdb.ui > > - in IPinProvider, what are all the IPresentationContext meant to be used > > for? They are not used in GdbPinProvider so I don't see why they are > > needed > For other provider that cares about which instance of the view it is pinned to. But why did you choose IPresentationContext? It is an internal class to the platform... Is using IWorkbenchPart or even IViewPart too limiting? > > Note that I have not seen any bugs that could show that this is a problem, so > > I cannot know for sure... > Now I see what you mean, I believe you are right. > DebugContextPinProvider.fActiveContext should be updated, I have made the > change to update this member in the delegateEvent. Let me know if this is > correct or not. Makes sense. I hadn't realize it would be such a simple fix. Great! > > > DebugEventFilterService > > > 12- in removeDebugEventFilter() you call contextProvider.delegateEvent() > > I'm still now clear about this one, but it is the same issue as the point > > above about getActiveContext() so let's see what happens there first. > Do we still need to address this concern? That is now fixed with the point right above. > > - Multiple context pinning is not working well. For example, with a multi- > > selection, I can pin to a GdbLaunch, which I normally cannot. I can also > > pin to a JDT context if I also select a CDT context. > Fixed, I messed this up in previous patch. Please give this patch a try. Looks good! - In PinHandleElement, did you mean to make the two members package-private or should they have the 'private' keyword? - I really like that you introduced IPinElementHandle, things are much clearer with it. I think that this concludes my comments for the pin and clone, without the decorator. To go faster, here is what I suggest: how about using the patch called "Rework to address Marc's comment #29 (minor update)", addressing the comments I just posted, and then committing it without the decorator. After that, if you can post the decorator patch as an increment to the new HEAD, it will make my review faster, and we'll have the bigger part of pin and clone already committed. What do you say? Great work by the way! Created attachment 186845 [details] Patch reviewed by Marc (In reply to comment #34) > Maybe. Does sound abnormal. I'll file a bug after the patch is committed, so that I can refer to HEAD in the bug description. > open a bug for this to explain that it is a workaround for the > ServiceRegistration potential issue. I'll file a bug after the patch is committed. > Sorry, still some typos (some of which I didn't even see before). Should be > // if there is a view without a secondary id, then get the next available id. Fixed. > You still need to check for > if (pinProvider == null) return false; > to avoid going through the loop for nothing. And then you can remove the > check for != null inside the loop. I intended to fall back to the "else if" if the pinProvider is null, this will enable CDebugElement client (CDI); which is currently not supported (comment out in PinCloneUtils#isPinnable). > I don't see other things left, except the new one I mentioned at the beginning > of this mail. Will you open bugs to track those two? Yes. > You already iterate over all views in the init() method. So I was originally > thinking of addind the following: > ... > The second line really should use PinCloneUtils.isClonedPart() but that > method requires an IViewPart. But I think we can change the method > to take an IViewRef, or maybe have a second method isClonedPart() with > an IViewRef. what do you think? Great! I think this will work and less code too. > Can you remove the two remaining references to IPinHandleLabelProvider in > GdbAdapterFactory? > Also, I believe you can remove the change in plugin.xml of dsf.gdb.ui Removed. > But why did you choose IPresentationContext? It is an internal class to the > platform... > Is using IWorkbenchPart or even IViewPart too limiting? I was copying how DSF and platform uses the PresentationContext to pass things around. I guess you are right, there isn't anything that we can't get from the IWorkbenchPart. I changed the interface to use IWorkbenchPart instead of IViewPart, allow IWorkbenchPart sub-classes, i.e IDisassemblyPart, IEditorPart, etc... to support pinning feature. > - In PinHandleElement, did you mean to make the two members package-private or > should they have the 'private' keyword? Private keyword. Updated. > I think that this concludes my comments for the pin and clone, without the > decorator. To go faster, here is what I suggest: how about using the patch > called "Rework to address Marc's comment #29 (minor update)", addressing the > comments I just posted, and then committing it without the decorator. > After that, if you can post the decorator patch as an increment to the new > HEAD, it will make my review faster, and we'll have the bigger part of pin and > clone already committed. What do you say? This is the right thing to do. I'll commit the new patch I just posted. Before I commit, do you know if CDT HEAD is built against Eclipse 3.7 M4? I posted a message on the mailing list, but didn't get an answer yet. We need to have CDT to align with Eclipse 3.7 M4 for the new platform API. (In reply to comment #35) > This is the right thing to do. I'll commit the new patch I just posted. Before > I commit, do you know if CDT HEAD is built against Eclipse 3.7 M4? I posted a > message on the mailing list, but didn't get an answer yet. We need to have CDT > to align with Eclipse 3.7 M4 for the new platform API. I don't know for sure but I believe it is. Reading the following where Vivian says she was going to do a build on M4: http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg20882.html (In reply to comment #35) > Created attachment 186845 [details] > Patch reviewed by Marc Looks great. I'm looking forward to seeing it committed! Thanks for addressing my picky comments. > > You still need to check for > > if (pinProvider == null) return false; > > to avoid going through the loop for nothing. And then you can remove the > > check for != null inside the loop. > > I intended to fall back to the "else if" if the pinProvider is null, this will > enable CDebugElement client (CDI); which is currently not supported (comment > out in PinCloneUtils#isPinnable). Sorry, I completely missed that logic. You are right. Looks good in the last patch. - I'm getting NPEs, because in GdbPinProvider#pin we should not set pinContext = execDmc if execDmc is null. You can reproduce it by pinning to the process entry in the DV and then changing the active context. - Now that I understand the feature better, I'm confused again about why we choose to pin to a thread instead of a frame? Since we pin to a context, if we were to pin to a frame context, we would not actually pin to the frame itself, but to the frame position. That means that if the user pins to the top stack frame, then the pinned view would keep updating to show the contents of the top frame. Isn't that what we ultimately want? Also, pinning to a thread and pinning to a frame, as the patch is right now, causes the same result, so if we were to allow to pin to a stack frame, we could still use the thread to get the current pin behavior. The problem with this approach is that it may not be obvious to a user to use the thread to pin. Toni, Pawel, this patch has a small change to the disassembly view in the dsf.ui plugin, in case you want to make sure it is ok. Created attachment 186899 [details]
DisassemblyPart patch
There was a conflict with my previous check-in to DisassemblyPart. Please use this patch in replacement for the change in the main patch. Otherwise the change seems fine.
I noticed that in DebugEventFilterService an event is fired from within a synchronized block. This is potentially deadlock-prone. I would recommend to fire the event outside the synchronized block.
Created attachment 186903 [details]
DisassemblyPart patch (2)
Sorry, my last patch was incomplete.
Created attachment 186918 [details] Patch reviewd by Marc and Toni (In reply to comment #38) > - I'm getting NPEs, because in GdbPinProvider#pin we should not set pinContext > = execDmc if execDmc is null. You can reproduce it by pinning to the process > entry in the DV and then changing the active context. Fixed. > - Now that I understand the feature better, I'm confused again about why we > choose to pin to a thread instead of a frame? Since we pin to a context, if > we were to pin to a frame context, we would not actually pin to the frame > itself, but to the frame position. That means that if the user pins to the > top stack frame, then the pinned view would keep updating to show the > contents of the top frame. Isn't that what we ultimately want? Depends on which view we are referring to. For example, variable, expression, and register make sense to pin to frame position, memory and disassembly might not. You lose the ability to switch between frame if it is pinned to frame. The workbench part is passed into the pin API, we can make this change in the GdbPinProvider. Should we do this after I submit the patch, so that we can review the incremental changes? > Also, pinning to a thread and pinning to a frame, as the patch is right now, > causes the same result, so if we were to allow to pin to a stack frame, we > could still use the thread to get the current pin behavior. The problem with > this approach is that it may not be obvious to a user to use the thread to > pin. The overlay pin icon in the debug view will help in this case, as well as the view description for the pinned view. (In reply to comment #40) > There was a conflict with my previous check-in to DisassemblyPart. Please use > this patch in replacement for the change in the main patch. Otherwise the > change seems fine. Merged the change to the new patch. > I noticed that in DebugEventFilterService an event is fired from within a > synchronized block. This is potentially deadlock-prone. I would recommend to > fire the event outside the synchronized block. I have modified the code to do what you have suggested. (In reply to comment #42) > > - Now that I understand the feature better, I'm confused again about why we > > choose to pin to a thread instead of a frame? Since we pin to a context, if > we were to pin to a frame context, we would not actually pin to the frame > > itself, but to the frame position. That means that if the user pins to the > > top stack frame, then the pinned view would keep updating to show the > > contents of the top frame. Isn't that what we ultimately want? > > Depends on which view we are referring to. For example, variable, expression, > and register make sense to pin to frame position, memory and disassembly might > not. You lose the ability to switch between frame if it is pinned to frame. The > workbench part is passed into the pin API, we can make this change in the > GdbPinProvider. Should we do this after I submit the patch, so that we can > review the incremental changes? Yes, please go ahead a commit :-) I think the feature is in great shape. We can discuss this idea after. *** cdt cvs genie on behalf of pchuong *** Bug 331781 - Pin and Clone support [+] pin.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/icons/pin.gif?root=Tools_Project&revision=1.1&view=markup [+] open_new.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/icons/open_new.gif?root=Tools_Project&revision=1.1&view=markup [*] MemoryBrowser.java 1.36 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/src/org/eclipse/cdt/debug/ui/memory/memorybrowser/MemoryBrowser.java?root=Tools_Project&r1=1.35&r2=1.36 [*] plugin.properties 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.properties?root=Tools_Project&r1=1.1&r2=1.2 [*] plugin.xml 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml?root=Tools_Project&r1=1.7&r2=1.8 [+] ViewIDCounterManager.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/ViewIDCounterManager.java?root=Tools_Project&revision=1.1&view=markup [+] DebugEventFilterService.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/DebugEventFilterService.java?root=Tools_Project&revision=1.1&view=markup [+] PinCloneUtils.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/PinCloneUtils.java?root=Tools_Project&revision=1.1&view=markup [+] DebugContextPinProvider.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/DebugContextPinProvider.java?root=Tools_Project&revision=1.1&view=markup [+] PinDebugContextActionDelegate.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/PinDebugContextActionDelegate.java?root=Tools_Project&revision=1.1&view=markup [+] OpenNewViewActionDelegate.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/OpenNewViewActionDelegate.java?root=Tools_Project&revision=1.1&view=markup [+] IPinProvider.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/ui/IPinProvider.java?root=Tools_Project&revision=1.1&view=markup [*] CDebugUIPlugin.java 1.71 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/ui/CDebugUIPlugin.java?root=Tools_Project&r1=1.70&r2=1.71 [+] PinElementHandle.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/ui/PinElementHandle.java?root=Tools_Project&revision=1.1&view=markup [*] MANIFEST.MF 1.30 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.29&r2=1.30 [*] plugin.xml 1.254 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/plugin.xml?root=Tools_Project&r1=1.253&r2=1.254 [+] pin.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/icons/elcl16/pin.gif?root=Tools_Project&revision=1.1&view=markup [+] open_new.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/icons/elcl16/open_new.gif?root=Tools_Project&revision=1.1&view=markup [+] pin.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/icons/pin.gif?root=Tools_Project&revision=1.1&view=markup [+] open_new.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/icons/open_new.gif?root=Tools_Project&revision=1.1&view=markup [*] plugin.properties 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.properties?root=Tools_Project&r1=1.14&r2=1.15 [*] plugin.xml 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.xml?root=Tools_Project&r1=1.24&r2=1.25 [*] DisassemblyPart.java 1.44 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyPart.java?root=Tools_Project&r1=1.43&r2=1.44 [+] GdbPinProvider.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/GdbPinProvider.java?root=Tools_Project&revision=1.1&view=markup [*] GdbAdapterFactory.java 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/GdbAdapterFactory.java?root=Tools_Project&r1=1.16&r2=1.17 [*] plugin.properties 1.125 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/plugin.properties?root=Tools_Project&r1=1.124&r2=1.125 I have committed the patch with filename "331781_cdt_pin_clone_support_Marc_comment_38_40.patch". I will open bugzilla entries to track these three known issues and mark this bug fixed. Issues 1. ServiceRegistration issue - see comment #34 2. Tooltip bug for minimized view - see comment #34 3. Pin context - see comment #43 Fixed, committed to HEAD. (In reply to comment #46) > Fixed, committed to HEAD. Congrats Patrick! Nice work. Patrick, I just realized that when we terminate a launch, views pinned to that launch still show the old data. Is this by design or did we miss a use-case? (In reply to comment #48) > Patrick, I just realized that when we terminate a launch, views pinned to that > launch still show the old data. Is this by design or did we miss a use-case? I design it to work this way. Is this something that you like to change? We'll need to have a new event or callback from the model to notify the view to send an empty selection debug context change event. (In reply to comment #49) > (In reply to comment #48) > > Patrick, I just realized that when we terminate a launch, views pinned to that > > launch still show the old data. Is this by design or did we miss a use-case? > > I design it to work this way. > > Is this something that you like to change? We'll need to have a new event or > callback from the model to notify the view to send an empty selection debug > context change event. It does look weird to have views showing data for launches that don't exist. What if DebugEventFilter implemented ILaunchesListener2. When launchesTerminated is called we could have some logic to check if the pin handles still belong to active session, or something like that to tell us we need to send this empty selection debug context change event. I suggest a new bug for this. |
