Community
Participate
Working Groups
Debug view should show icon overlay for pinned debug context.
Created attachment 187050 [details] Patch for dsf-gdb A state change event is introduced at the launch VM and is fired when the element is pin, unpin and re-start.
Created attachment 187051 [details] Icon files with overlay decorator
Marc, can you review? The delta generation and StateChangedEvent is in dsf, perhaps Pawel should also review this patch as well.
So, no more color differentiation for the overlay?
Nice patch. Some comments below: 1-Since the new icons could be used for CDI and other CDT debuggers, how about adding them to the org.eclipse.cdt.ui plugin and class CDTSharedImages? GdbPinProvider 2- session.addServiceEventListener must be called from the executor. It shoud have thrown an assertion for you. Make sure you add -ea to your vm args. 3- you must call session.removeServiceEventListerner in a disposed() method, which is called from GdbAdaptorFactory. Mimic SteppingController in GdbAdaptorFactory. 4- What is public void handleEvent(IStartedDMEvent event) meant to do? Can you put a comment. How come we didn't need it before? 5- got an NPE in GdbPinProvider:93 where session was null. This happened as I was terminating the launch. My guess is that it will go away once you have a disposed() method. But you probably want to store the session globally instead of fetching it each time. 6- does isPinnedTo() really need to be synchronized 7- I think you should move the change from ILaunchVMConstants to IGdbLaunchVMConstants, since the corresponding changes are in ContainerVMNode/ThreadVMNode instead of in DSF's AbstractContainerVMNode/AbstractThreadVMNode. 8- The new generic StateChangedEvent is a nice easy way to refresh labels, but we need Pawel to ok it. If he prefers to not have this in DSF, then move it to DSF-GDB.
Created attachment 187317 [details] Rework to address Marc's comment #5 (In reply to comment #4) > So, no more color differentiation for the overlay? Without platform overlay support, much more work is involved to create the icons with different overlay. I have addressed this in the latest patch, with 3 colors limit, you will see why when you look at the patch :( (In reply to comment #5) > 1-Since the new icons could be used for CDI and other CDT debuggers, how about > adding them to the org.eclipse.cdt.ui plugin and class CDTSharedImages? Make sense, I didn't realize there is a shared image class. > GdbPinProvider > 2- session.addServiceEventListener must be called from the executor. Fixed, thanks for the vmarg advice. > 3- you must call session.removeServiceEventListerner in a disposed() method, > which is called from GdbAdaptorFactory. Mimic SteppingController in > GdbAdaptorFactory. Yep, didn't un register. > 4- What is public void handleEvent(IStartedDMEvent event) meant to do? > Can you put a comment. How come we didn't need it before? It meant to rewire the pinned debug context again when user relaunch the same session. Before, we were only comparing the pinned label. Now, I need the DMContext to fire event to update the label. > 5- got an NPE in GdbPinProvider:93 where session was null. This happened as I > was terminating the launch. I don't see NPE after I unregister the listener and updating the patch. > 6- does isPinnedTo() really need to be synchronized No, I don't think so. > 7- I think you should move the change from ILaunchVMConstants to > IGdbLaunchVMConstants, since the corresponding changes are in > ContainerVMNode/ThreadVMNode instead of in DSF's > AbstractContainerVMNode/AbstractThreadVMNode. Yes, I think so too. > 8- The new generic StateChangedEvent is a nice easy way to refresh labels, but > we need Pawel to ok it. If he prefers to not have this in DSF, then move > it to DSF-GDB. Waiting for pawel to comment.
Created attachment 187318 [details] New icon files Icon files replacement.
Created attachment 187320 [details] Rework to address Marc's comment #5 (fixed patch) Found a bug in the ContainerVMNode, it doesn't update the pin icon when target is running.
(In reply to comment #6) > Created attachment 187317 [details] > Rework to address Marc's comment #5 > > (In reply to comment #4) > > So, no more color differentiation for the overlay? > > Without platform overlay support, much more work is involved to create the > icons with different overlay. I have addressed this in the latest patch, with 3 > colors limit, you will see why when you look at the patch :( I personally have the impression that having the colors is helpful. I see that it is not as nice as when supported by the platform. Is the technique you used the one that Pawel recommended? We can discuss the (soft) limitation to three color at the multi-core meeting to see if people want the color or not, based on this approach. A couple more comments on the patch. IPinProvider - found a trick to allow others to extend an enum. Can you define an empty interface called 'IPinColor'. Have Color implements IPinColor. Then, use IPinColor instead of Color all the time (except when you actually refer to GREEN, RED and BLUE). That way, if someone wants to define more colors, they can define a new enum that implements IPinColor and they can use all the old colors, and the new ones. - shouldn't PinCloneUtils#getPinColor() call IPinProvider#isPinnedTo() instead of simply comparing the contexts? I think you actually need to call DebugContextPinProvider#isPinnedTo(), but to be able to do that you will need to convert it to a static method and move it to PinCloneUtils, which I think is fine. - GdbPinProvider, in dispose() you must catch RejectedExecutionException in case the session is already gone. I saw this exception during testing. - ThreadVMNode, the three images for pinning use the CONTAINER image instead of the THREAD - ContainerVMNode, in the three new SUSPENDED cases, in isEnabled() you don't need to check PROP_IS_SUSPENDED since we know it is suspended. In fact, in fact, in setPropertyNames, that property is not even set. You've done this properly in ThreadVMNode - IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED is either not needed or is not being used properly. That property is always being set to true in updatePropertiesInSessionThread (both for container and thread). That means that the property always indicates that the context is pinned, which is not correct. But things still work properly. This is because of the pin_color property. This property happens to be set to null by PinCloneUtils.getPinColor(), which makes the pin icon not be used. So, eiter we get rid of IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED and only use the color and count ont this null value, or we have to set IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED by using isPinnedTo() (I think). I personally suggest we keep IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED, in case we want to disable the colors. I still need to go over GdbPinProvider in more detail. I'll try to find more time this afternoon.
I looked deeper into GdbPinProvider and there may be some issues. I recommend looking at issues 9-10-11 first because they may change the code and invalidate the other comments :-) GdbPinProvider 1- for gsPinnedHandles, can you use Collections.synchronizedSet(new HashSet(...)) and remove the synchronized keyworks from the methods? 2- Can you make fSession final. So we don't have to synchronize its access 3- can you make getExecutionDmc() and getProcessDmc() private? 4- in getData(), inside Query#execute, you must call rm.done() if processes == null (I missed this before) 5- in pin(), you have to modulo the secondary id before accessing the color array color = Color.values()[Integer.parseInt(id) % Color.values().length]; 6- in handleEvent, why do you start a job inside the for loop instead of putting the loop inside the job? 7- in handleEvent, should we really use the highest priority (INTERACTIVE)? There is a DECORATE priority... 8- I still saw java.util.concurrent.RejectedExecutionException at java.util.concurrent.ThreadPoolExecutor$AbortPolicy.rejectedExecution(ThreadPoolExecutor.java:1768) at java.util.concurrent.ThreadPoolExecutor.reject(ThreadPoolExecutor.java:767) at java.util.concurrent.ScheduledThreadPoolExecutor.delayedExecute(ScheduledThreadPoolExecutor.java:215) at java.util.concurrent.ScheduledThreadPoolExecutor.schedule(ScheduledThreadPoolExecutor.java:397) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor.schedule(DefaultDsfExecutor.java:434) at java.util.concurrent.ScheduledThreadPoolExecutor.execute(ScheduledThreadPoolExecutor.java:464) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor.execute(DefaultDsfExecutor.java:458) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.dispose(GdbPinProvider.java:65) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbAdapterFactory$SessionAdapterSet.dispose(GdbAdapterFactory.java:289) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbAdapterFactory.disposeAdapterSet(GdbAdapterFactory.java:318) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbAdapterFactory.launchesRemoved(GdbAdapterFactory.java:394) you should catch the rejectedExecution in dispose() like SteppingController#dispose() 9- Try this. Launch a session and pin to the stack frame. Then launch the exact same launch config. You should see that the stack frame of the second session shows the pinned icon as well (because of the handleEvent logic). Furthermore, if you un-pin at that time, the icons don't disappear anymore. 10- If I have two identical sessions (I launch the same thing twice), and I pin to one of them, because isPinnedTo compares labels, the view will actually think it is pinned to either one of the two sessions. That means that if I change the selection to the other session, the view will change. However, the icon only shows for one of them. I'm not sure if the problem is the icon not showing on both, or if the view should not be pinned on both. 11- Related to 9 and 10, when a new launch starts and has the same getCombinedLabels() as an existing launch, handleEvent() will call handle.setDebugContext() with the context of the new launch. However, the old launch is still there and the view is still pinned to it. That could be the bug of #9. About 9-10-11, the first question we need to answer is what do you want to do when a view is pinned to one launch, and the same launch is started a second time. Do we pin to both or not? Once we have decided that, we can figure out the way to fix the issues.
Marc, this is a known issue. I have it on my TODO list. The pin icon doesn't work well with multiple sessions (same launch config) at the same time. I think we need to use the session id to associate with the pin context (label) and only show the pin icon for that session. If the user terminate all the sessions and relaunch, than we'll attach to the first session and ignore subsequence launchs. If the user terminate the pinned session, than the other session(s) won't get affected. What do you think?
(In reply to comment #11) > Marc, this is a known issue. I have it on my TODO list. The pin icon doesn't > work well with multiple sessions (same launch config) at the same time. I think > we need to use the session id to associate with the pin context (label) and > only show the pin icon for that session. If the user terminate all the sessions > and relaunch, than we'll attach to the first session and ignore subsequence > launchs. If the user terminate the pinned session, than the other session(s) > won't get affected. What do you think? Sounds good to me. You can use the sessionId, or you could use the ICommandControlDMContext which is unique per session (at least it is for GDB).
Another question that I don't know how to solve yet. In the getCombinedLabels method in GdbPinProvider, the call to getData get stuck if it is calling from the executor thread. Is there a way to get the thread name and thread id without going through the IProcess service? This is the reason that I didn't use isPinnedTo in the VMNode.
(In reply to comment #13) > Another question that I don't know how to solve yet. In the getCombinedLabels > method in GdbPinProvider, the call to getData get stuck if it is calling from > the executor thread. Is there a way to get the thread name and thread id > without going through the IProcess service? This is the reason that I didn't > use isPinnedTo in the VMNode. Maybe using the new ImmediateInDsfExecutor to run the query? It will only use the executor if it is not already in it.
Created attachment 188074 [details] Rework to address Marc's comment #9 & #10 (In reply to comment #9) > I personally have the impression that having the colors is helpful. I see > that it is not as nice as when supported by the platform. Is the technique > you used the one that Pawel recommended? No. I didn't get an example from Pawel. > IPinProvider > - found a trick to allow others to extend an enum. Can you define an empty > interface called 'IPinColor'. Since we want to be able to extended by other client, I created an interface to allow other pin provider to provide additional colors and override the toolbar icon when it is pinned. > - shouldn't PinCloneUtils#getPinColor() call IPinProvider#isPinnedTo() instead > of simply comparing the contexts? Yes, after I made the change you have suggested by using ImmediateExecutor(), I was able to get the name of the process and thread from the session executor. > - GdbPinProvider, in dispose() you must catch RejectedExecutionException in > case the session is already gone. I saw this exception during testing. Fixed, I didn't realized we need to catch this exception. > - ThreadVMNode, the three images for pinning use the CONTAINER image instead > of the THREAD Fixed. > - ContainerVMNode, in the three new SUSPENDED cases, in isEnabled() you don't > need to check PROP_IS_SUSPENDED since we know it is suspended. Fixed, copy and paste error. > - IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED is either not needed or is > not being used properly. ... I kept the PROP_PINNED_CONTEXT_CHANGED and but make sure that the flag is set properly, even with the disabled colors I am not sure if we need it or not; we could treat it as a different color. (In reply to comment #10) > GdbPinProvider > 1- for gsPinnedHandles, can you use Collections.synchronizedSet(new > HashSet(...)) and remove the synchronized keyworks from the methods? Sure. Changed. > 2- Can you make fSession final. So we don't have to synchronize its access Yes, I also changed the code in the new patch. This field is only used for listening to service event. > 3- can you make getExecutionDmc() and getProcessDmc() private? Yes, done. > 4- in getData(), inside Query#execute, you must call rm.done() if processes == > null (I missed this before) Changed to use ImmediateExecutor(), can you take a look at the patch and see whether I use it correct or not? > 5- in pin(), you have to modulo the secondary id before accessing the color > array > color = Color.values()[Integer.parseInt(id) % Color.values().length]; Yeah, my bad. Fixed. > 6- in handleEvent, why do you start a job inside the for loop instead of > putting the loop inside the job? The reason for the job inside a loop is I don't want to schedule the job if there is no pin handle. I can go either way, but I leave it the way it is right now. Can you let me know which way is better and I'll make the change. > 7- in handleEvent, should we really use the highest priority (INTERACTIVE)? > There is a DECORATE priority... Yes, otherwise the icon takes a couple seconds to show up. > 8- I still saw > java.util.concurrent.RejectedExecutionException > at Fixed. Let me know if you still see any more RejectedExecutionException. > 9-10-11... I addressed multi-sessions and terminate/relaunch issues in this patch, let me know how it workout. I can change the behavior if the current one isn't suitable for GDB. TI only allows one debug session per launch configuration, so we don't have this issue. I'll let you decide what is best for GDB :), and I can adjust the behavior.
(In reply to comment #15) > (In reply to comment #9) > > I personally have the impression that having the colors is helpful. I see > > that it is not as nice as when supported by the platform. Is the technique > > you used the one that Pawel recommended? > No. I didn't get an example from Pawel. If it is ok with you, I suggest we wait until the multi-core debug work group meeting next week to ask Pawel about it. I really hope there is a less brute-force solution to this problem. I have put this item as the first thing on the agenda for the meeting.
I reviewed the latest patch and I think we are very close. Thanks for addressing all the issues. There are a couple of things left though: - The new patch is not working properly. Things seem very slow when I pin. But we can try this out again with the final version. - Also, I'm getting assertions. Don't forget to run with -ea. The reason for the assertion is that you are using the ImmediateExecutor in GdbPinProvider now, but that class is not running in the executor. The ImmediateExecutor should only be used if you know you are already in the DSF executor. So, you can use the ImmediateExecutor for the call to getExecutionData() inside the Query, but you need to execute the Query inside the DSF executor. If in some cases you may or may not be in the DSF executor already, you can use the ImmediateInDsfExecutor instead. - in PinDebugContextActionDelegate#updatePinContextColor can handle.getPinElementColorDescriptor(); return null? The javadoc does not say, so we are at risk of an NPE - The new PinCloneUtils#isPinnedTo() is the same (slight differences) as DebugContextPinProvider#isPinnedTo() I suggest DebugContextPinProvider#isPinnedTo() call the other one to avoid duplication. - I'm hesitant about the generic StateChangedEvent. I'm not sure this fits with other DSF events. I mean, why not have a generic event for all types of changes? I'd like to get Pawel's opinion at the multi-core meeting. - How come the StateChangedEvent is handled exactly the same way by both Container and Thread? Which one should refresh? Shouldn't we check the dmc to know if it is applicable?
Patrick, I'm sure you know this, but I just realized that even for a single session, if we pin different views to the same context, the pin icon kind of looses its value, no? If I pin a register view and a variable view to the same context, each view can have a different colored pin icon, but the debug view context will not be able to show both, so will probably show the default green one. Do you have any thoughts on that?
(In reply to comment #18) > Patrick, I'm sure you know this, but I just realized that even for a single > session, if we pin different views to the same context, the pin icon kind of > looses its value, no? > > If I pin a register view and a variable view to the same context, each view can > have a different colored pin icon, but the debug view context will not be able > to show both, so will probably show the default green one. I've been thinking about this because I feel it is a big limitation. I think I have a simple approach that is very user-friendly. When a view is pinned to a context, we should see if the same context already has a pin color associated with it and re-use the same color. That will mean that all views pinned to the same context will have the same pin color, which I think is what the user wants to see. I'm not sure how hard this will be in the code. Patrick, what do you think? Having a view pinned to multiple contexts at once may not fit well with this solution, but isn't true that a multi-pin is more rare than having multiple views pinned to one context?
I think it is a good idea, I'll investigate and will get back to you. As well as your previous comments. Regarding multi-context pinning, we can have a different view icon when the view is pinned to multi-context.
(In reply to comment #20) > I think it is a good idea, I'll investigate and will get back to you. As well > as your previous comments. > > Regarding multi-context pinning, we can have a different view icon when the > view is pinned to multi-context. That sounds like a good plan.
Created attachment 188704 [details] Rework to address Marc's comment #17 & #19 (In reply to comment #17) > - Also, I'm getting assertions. Don't forget to run with -ea. The reason for > the assertion is that you are using the ImmediateExecutor in GdbPinProvider > now, but that class is not running in the executor. The ImmediateExecutor > should only be used if you know you are already in the DSF executor. So, you > can use the ImmediateExecutor for the call to getExecutionData() inside the > Query, but you need to execute the Query inside the DSF executor. If in some > cases you may or may not be in the DSF executor already, you can use the > ImmediateInDsfExecutor instead. Yeah, I keep missing the -ea. Can you look over the getData() in GDBPinProvider again, and make sure I am using the executor correctly? > - in PinDebugContextActionDelegate#updatePinContextColor can > handle.getPinElementColorDescriptor(); return null? The javadoc does not say, > so we are at risk of an NPE I updated the javadoc and make sure that I check for nullpointer. > - The new PinCloneUtils#isPinnedTo() is the same (slight differences) as > DebugContextPinProvider#isPinnedTo() I suggest > DebugContextPinProvider#isPinnedTo() call the other one to avoid duplication. yep, fixed. > - I'm hesitant about the generic StateChangedEvent. I'm not sure this fits > with other DSF events. I mean, why not have a generic event for all types of > changes? I'd like to get Pawel's opinion at the multi-core meeting. This is an UI refresh event, not a model change event. It basically notify the tree node to update it's label and icon. > - How come the StateChangedEvent is handled exactly the same way by both > Container and Thread? Which one should refresh? Shouldn't we check the dmc > to know if it is applicable? The notification is sent to the dmc that needs to update. Shouldn't need to do additional checking, unless I missed something here. (In reply to comment #19) > I've been thinking about this because I feel it is a big limitation. > I think I have a simple approach that is very user-friendly. > When a view is pinned to a context, we should see if the same context already > has a pin color associated with it and re-use the same color. That will mean > that all views pinned to the same context will have the same pin color, which > I think is what the user wants to see. Can you test the new patch, let me know what you think about the reusing the same color code. Pay attention to the overlay icon when you unpin view, the color reverts back to it's default color; as well as the toolbar icon.
Created attachment 188705 [details] Icon files with multi-pin toolbar icon added multi-pin icon
(In reply to comment #22) > Yeah, I keep missing the -ea. Can you look over the getData() in GDBPinProvider > again, and make sure I am using the executor correctly? You should dispose of the tracker in a finally clause in case an exception is thrown. Inside the query, you need to add else { rm.setData(null); rm.done() } for the case where (processes == null) > Can you test the new patch, let me know what you think about the reusing the > same color code. Pay attention to the overlay icon when you unpin view, the > color reverts back to it's default color; as well as the toolbar icon. I'll do that more thoroughly tomorrow.
On terminate I saw this after 2 seconds: java.util.concurrent.TimeoutException at org.eclipse.cdt.dsf.concurrent.Query.get(Query.java:128) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.getData(GdbPinProvider.java:152) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.getCombinedLabels(GdbPinProvider.java:175) at org.eclipse.cdt.dsf.gdb.internal.ui.GdbPinProvider.isPinnedTo(GdbPinProvider.java:318) at org.eclipse.cdt.debug.internal.ui.pinclone.PinCloneUtils.isPinnedTo(PinCloneUtils.java:252) at org.eclipse.cdt.debug.internal.ui.pinclone.DebugContextPinProvider.isPinnedTo(DebugContextPinProvider.java:97) at org.eclipse.cdt.debug.internal.ui.pinclone.DebugEventFilterService$DebugEventFilter.debugContextChanged(DebugEventFilterService.java:51)
Nice work. Last comments below, along with comment 24 and comment 25, and I think it is good to commit. - Can you explain what useMultiPinImage() does? I don't understand the algo. - I'm still not sure about the multi-context pin coloring, but I think we spent enough time on it already. We can see how things work out with the current solution. I'll just focus on the single context pin scenario, with multiple views of course. - for AbstractThreadVMNode#buildDelta the new code should be (note that we have already checked that the dmc is not a containerDmc): } else if (e instanceof StateChangedEvent) { parentDelta.addNode(createVMContext(dmc), IModelDelta.STATE); rm.done(); - for AbstractContainerVMNode#buildDelta the new code should be: } else if (e instanceof StateChangedEvent) { // If there is a state change needed on the container, update the container if (dmc instanceof IContainerDMContext) { parentDelta.addNode(createVMContext(dmc), IModelDelta.STATE); } - is the name IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED correct? The way it is used, I think it should be called IGdbLaunchVMConstants.PROP_PINNED_CONTEXT - in GdbPinProvider#isPinnedTo, I think the line if (dmc.getSessionId() == hDmc.getSessionId()) should use equals() instead of ==
Created attachment 188932 [details] Rework to address Marc's comment #24 & #25 & #26 Marc, can you do a final review of this patch. I don't really like how the pin color is assigned from the last patch, I made some adjustment. Now, the color is based off the debug context label. You will need the new icons as well. I renamed some icons, how do I remove files from cvs? Hopefully this patch can be committed to HEAD. (In reply to comment #26) > - Can you explain what useMultiPinImage() does? I don't understand the algo. This algorithm detects whether multiple toolbar icons or multipe overlay colors are set for the pin handles (a view), if so then use the double pin icon. > - I'm still not sure about the multi-context pin coloring, but I think we > spent enough time on it already. We can see how things work out with the > current solution. I'll just focus on the single context pin scenario, with > multiple views of course. I have updated the patch to simplified pin color logic, the pin color is now based on the debug context. This is more closer than what you were asking for. i.e view uses the same pin color when pin to the same context. > - for AbstractThreadVMNode#buildDelta the new code should be (note that we > fixed. > - for AbstractContainerVMNode#buildDelta the new code should be: fixed > - is the name IGdbLaunchVMConstants.PROP_PINNED_CONTEXT_CHANGED correct? The > way it is used, I think it should be called > IGdbLaunchVMConstants.PROP_PINNED_CONTEXT renamed. > - in GdbPinProvider#isPinnedTo, I think the line > if (dmc.getSessionId() == hDmc.getSessionId()) > should use equals() instead of == - fixed. (In reply to comment #25) > On terminate I saw this after 2 seconds: This is due to the query is set to 2 seconds timeout. I removed this condition, hopefully, there is nothing that blocks the main thread when querying the label from gdb. (In reply to comment #24) > Inside the query, you need to add > else { > rm.setData(null); > rm.done() > } > for the case where (processes == null) fixed.
Created attachment 188933 [details] New icon files with gray color view pin icon
(In reply to comment #27) Nice work Patrick. 5 minor issues below and then I think it's good to commit. Thanks for your patience. I like the new gray pinned icon. It always bothered me that the green pin was used for both unpinned and pinned (although the button is pressed in the GUI, it was still a bit confusing). > I renamed some icons, how do I remove files from cvs? Hopefully this patch can > be committed to HEAD. I think you just delete them in Eclipse and commit the whole plugin. > (In reply to comment #26) > > - Can you explain what useMultiPinImage() does? I don't understand the algo. > This algorithm detects whether multiple toolbar icons or multipe overlay colors > are set for the pin handles (a view), if so then use the double pin icon. Ok, I get it. 1- Should the line: if (imageDesc != null && imageDesc.equals(descImageDesc)) actually be if (imageDesc != null && !imageDesc.equals(descImageDesc)) > > - I'm still not sure about the multi-context pin coloring, but I think we > > spent enough time on it already. We can see how things work out with the > > current solution. I'll just focus on the single context pin scenario, with > > multiple views of course. > I have updated the patch to simplified pin color logic, the pin color is now > based on the debug context. This is more closer than what you were asking for. > i.e view uses the same pin color when pin to the same context. Very nice! I like that much better than the modulo of the secondary id. I think it is a better user-experience now. GdbPinColorTracker 2- you should make the default constructor private for a singleton class 3- do we need to do synchronization for gsColorBuckets? Maybe just use Collections.synchronized 4- removeRef() is never being called. I think it should be called from GdbPinProvider.unpin() > (In reply to comment #25) > > On terminate I saw this after 2 seconds: > This is due to the query is set to 2 seconds timeout. I removed this condition, > hopefully, there is nothing that blocks the main thread when querying the label > from gdb. If the 2 second timeout was hitting, it means the query was not completing. Removing the timeout will just leave it dangling. But actually, the reason for the timeout is an assert in the getExecutionData() call. java.lang.AssertionError: Don't have entry for process ID: 8449 at org.eclipse.cdt.dsf.gdb.service.GDBProcesses_7_0.getExecutionData(GDBProcesses_7_0.java:645) This is a separate issue. 5- Please go ahead and put back your two seconds timeout.
*** cdt cvs genie on behalf of pchuong *** Bug 334566 - [multicore][Pin&Clone] Add support for icon overlay in the debug view [*] IGdbLaunchVMConstants.java 1.4 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/viewmodel/launch/IGdbLaunchVMConstants.java?root=Tools_Project&r1=1.3&r2=1.4 [*] ThreadVMNode.java 1.8 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/viewmodel/launch/ThreadVMNode.java?root=Tools_Project&r1=1.7&r2=1.8 [*] ContainerVMNode.java 1.5 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/viewmodel/launch/ContainerVMNode.java?root=Tools_Project&r1=1.4&r2=1.5 [*] GdbPinProvider.java 1.2 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&r1=1.1&r2=1.2 [*] GdbAdapterFactory.java 1.19 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.18&r2=1.19 [+] GdbPinColorTracker.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/GdbPinColorTracker.java?root=Tools_Project&revision=1.1&view=markup [*] IPinProvider.java 1.2 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&r1=1.1&r2=1.2 [*] PinElementHandle.java 1.2 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&r1=1.1&r2=1.2 [*] PinCloneUtils.java 1.2 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&r1=1.1&r2=1.2 [*] DebugContextPinProvider.java 1.2 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&r1=1.1&r2=1.2 [*] MANIFEST.MF 1.31 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.30&r2=1.31 [*] plugin.xml 1.255 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/plugin.xml?root=Tools_Project&r1=1.254&r2=1.255 [+] toolbar_pinned.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/icons/elcl16/toolbar_pinned.gif?root=Tools_Project&revision=1.1&view=markup [-] 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&view=markup [*] PinDebugContextActionDelegate.java 1.2 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&r1=1.1&r2=1.2 [+] thread_obj_b.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/thread_obj_b.gif?root=Tools_Project&revision=1.1&view=markup [+] debugts_obj_g.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugts_obj_g.gif?root=Tools_Project&revision=1.1&view=markup [+] debugts_obj_r.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugts_obj_r.gif?root=Tools_Project&revision=1.1&view=markup [+] debugts_obj_b.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugts_obj_b.gif?root=Tools_Project&revision=1.1&view=markup [+] debugt_obj_g.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugt_obj_g.gif?root=Tools_Project&revision=1.1&view=markup [+] toolbar_pinned_multi.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/toolbar_pinned_multi.gif?root=Tools_Project&revision=1.1&view=markup [+] toolbar_pinned_b.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/toolbar_pinned_b.gif?root=Tools_Project&revision=1.1&view=markup [+] debugt_obj_r.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugt_obj_r.gif?root=Tools_Project&revision=1.1&view=markup [+] threads_obj_b.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/threads_obj_b.gif?root=Tools_Project&revision=1.1&view=markup [+] toolbar_pinned_g.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/toolbar_pinned_g.gif?root=Tools_Project&revision=1.1&view=markup [+] toolbar_pinned.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/toolbar_pinned.gif?root=Tools_Project&revision=1.1&view=markup [+] threads_obj_g.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/threads_obj_g.gif?root=Tools_Project&revision=1.1&view=markup [+] thread_obj_r.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/thread_obj_r.gif?root=Tools_Project&revision=1.1&view=markup [+] toolbar_pinned_r.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/toolbar_pinned_r.gif?root=Tools_Project&revision=1.1&view=markup [+] debugt_obj_b.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/debugt_obj_b.gif?root=Tools_Project&revision=1.1&view=markup [+] threads_obj_r.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/threads_obj_r.gif?root=Tools_Project&revision=1.1&view=markup [+] thread_obj_g.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/icons/obj16/thread_obj_g.gif?root=Tools_Project&revision=1.1&view=markup [*] CDTSharedImages.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.ui/src/org/eclipse/cdt/ui/CDTSharedImages.java?root=Tools_Project&r1=1.4&r2=1.5 [+] toolbar_pinned.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/icons/toolbar_pinned.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&view=markup [*] plugin.xml 1.26 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.xml?root=Tools_Project&r1=1.25&r2=1.26 [+] StateChangedEvent.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/viewmodel/launch/StateChangedEvent.java?root=Tools_Project&revision=1.1&view=markup [*] AbstractContainerVMNode.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/viewmodel/launch/AbstractContainerVMNode.java?root=Tools_Project&r1=1.8&r2=1.9 [*] AbstractThreadVMNode.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/viewmodel/launch/AbstractThreadVMNode.java?root=Tools_Project&r1=1.12&r2=1.13 [+] toolbar_pinned.gif http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/icons/toolbar_pinned.gif?root=Tools_Project&revision=1.1&view=markup [-] 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&view=markup [*] plugin.xml 1.10 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.9&r2=1.10
Addressed Marc's comment #29 and committed the patch to HEAD.
(In reply to comment #31) > Addressed Marc's comment #29 and committed the patch to HEAD. Is the string you use for addRef and for removeRef the same? I think it is different. For addRef you added a '.' between the sessionId and the label, no?
I tried out the patch and it looks great. Nice job, Patrick! Should the pinned states be saved somewhere like the workspace or launch configuration? For example, consider this use case: 1) launch a debug configuration 2) open the disassembly view 3) pin the disassembly view to core 0 4) clone the disassembly view 5) pin the cloned disassembly view to core 1 6) exit Eclipse 7) start up Eclipse 8) launch the debug configuration. The two disassembly views are now unpinned. I'd expect them to be pinned to the same context and using the same colors too.
*** cdt cvs genie on behalf of pchuong *** Bug 334566 - [multicore][Pin&Clone] Add support for icon overlay in the debug view missing '.' for context. [*] GdbPinProvider.java 1.3 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&r1=1.2&r2=1.3
(In reply to comment #29) > If the 2 second timeout was hitting, it means the query was not completing. > Removing the timeout will just leave it dangling. > But actually, the reason for the timeout is an assert in the getExecutionData() > call. > java.lang.AssertionError: Don't have entry for process ID: 8449 > at > org.eclipse.cdt.dsf.gdb.service.GDBProcesses_7_0.getExecutionData(GDBProcesses_7_0.java:645) > This is a separate issue. I have fixed this separate issue with Bug 337927
