Community
Participate
Working Groups
The debug, variables, registers, and expressions view all implement the DSF view model to populate the flexible hierarchy views. The DSF view model introduced recently a new flexible hierarchy update interface IElementPropertiesProvider, and a corresponding PropertyLabelProvider. The debugger views should be updated to use these new APIs.
*** Bug 217928 has been marked as a duplicate of this bug. ***
Created attachment 128297 [details] Patch with fix.
In a week and a half sprint, I managed to complete this refactoring for 6.0 M6. It's a big change in how the labels for the debugger views are calculated, and in how the view data is cached. I really hope I didn't break too much in the process, but hopefully the M6 test cycle will help out weed out most problems. It is also a really big patch, so I don't expect any one person to review the whole thing, but hopefully between a few people most of the changes will be reviewed.
Created attachment 128392 [details] Remove superfluous update.done() I ran into AssertionFailedErrors in CountingRequestMonitor.done(). The culprit seems to be a superfluous call to update.done(). The patch removes this call. Otherwise, everything seems to work fine.
(In reply to comment #4) > Created an attachment (id=128392) [details] > Remove superfluous update.done() > > I ran into AssertionFailedErrors in CountingRequestMonitor.done(). The culprit > seems to be a superfluous call to update.done(). The patch removes this call. > Otherwise, everything seems to work fine. Thanks Toni. I applied a similar fix to yours, but I wonder why I haven't seen this assertion error myself. What were you doing when it happened?
(In reply to comment #5) > What were you doing when it happened? Never mind, I figured it out. You have to have the "delay stepping for view update on" to see this.
Created attachment 128471 [details] Fix for an NPE. I just came accross an NPE in the cache logic related to these changes. The following patch fixes it. Committed.
Created attachment 128484 [details] Debug view does not always refresh I'm getting some bad behaviour after updating to these changes. The Debug View sometime seems to get stuck. If you look at the attached screenshot you can notice: 1- in the console, the last *stopped event is on line 407, which is reflected in the editor, but the debug view shows 406. In fact, the debug view is stuck on 406. 2- when I selected another stack frame, the editor went to the wrong place (line 408 instead of 497) 3- the list of frames is wrong When I click on Refresh for the Debug view, the stack gets properly refreshed (although, in some cases, I had to minimize the debug view to trigger the proper refresh.) I will attach what the refreshed debug view looks like. I will look into it a little, but this falls into views, deltas and all that good stuff that I'm not very familiar with, so it may take me a long time to find something useful...
Created attachment 128485 [details] Debug view after it has been forced to refresh This is what the debug view should have looked like. I just had a thought. Could there be some problem with update policies where the update policy is on manual mode? My menu says it is on automatic, but maybe the menu is now being ignored?
Another note. Notice that in the first screenshot, when I selected the second stack frame, the editor seemed to go to the wrong line. However, after refreshing the debug view (seecond screenshot), we can see that the second stack frame is actually at line 408, which is where the editor went. So, the debug view seems to know about the correct frames, but is not showing them.
(In reply to comment #8) Thanks Marc, the problem likely originates somewhere from the bowels of the view model cache. It's not surprising that the source lookup is correct, because it does not re-use the view model cache. A few questions: 1) Is there anything special you do to reproduce this (step fast, step in, step over sleep, etc.)? 2) Do you have any exceptions in your log. 3) Do you have assertions turned on (-ea), this may cause additional exceptions to be logged. P.S. I guess we should move this to a new bug...
(In reply to comment #11) > 1) Is there anything special you do to reproduce this (step fast, step in, step > over sleep, etc.)? > 2) Do you have any exceptions in your log. > 3) Do you have assertions turned on (-ea), this may cause additional exceptions > to be logged. I didn't have -ea, so I added it. But I can no longer reproduce this problem. I'll try again later...
Created attachment 128609 [details] Removed duplicate code when filling in register DMData properties. There is a call to the fillRegisterDataProperties() and then several lines of code which fill in the same properties again. Looks like you created the fill routine and then forgot to delete the original property sets. Randy
Pawel I am also seeing the following in my development space which is 100% in sync with HEAD at this time. Register Groups dragged to the Expressions view or complex struct variables in the view when expanded show nothing in the expressions column. The NAME/VALUE/DESCRIPTION columns are working fine. Is anyone else seeing this. Randy
(In reply to comment #14) I'll take a look into it.
(In reply to comment #12) > (In reply to comment #11) > > > 1) Is there anything special you do to reproduce this (step fast, step in, step > > over sleep, etc.)? > > 2) Do you have any exceptions in your log. > > 3) Do you have assertions turned on (-ea), this may cause additional exceptions > > to be logged. > > I didn't have -ea, so I added it. > But I can no longer reproduce this problem. I'll try again later... I still can't reproduce it. Maybe it was a fluke... Just to be safe, can you point me to the class that you say must be the culprit. Maybe I can do a code review if I can figure out what this caching does.
I'll be honest and admit I didn't understand most of this propertyProvider stuff. So, I had a look at the code I was familiar with. Everything looks ok from that end. Some very minor comments on the new IDsfService.isActive() method: - For Bug 256798 "[services] Services can be called after they are shutdown", should we now have anyone using a service, call the new isActive() method, to make sure the service has not been shutdown? Instead of simply checking for null, which is not enough. - AbstractMIControl.isActive() now overrides IDsfService.isActive(). It should either be removed, or have the @Overrides annotation. - ICommandControlService can have its isActive() removed since it is now in IDsfService. That's all I got.
(In reply to comment #16) > I still can't reproduce it. Maybe it was a fluke... > Just to be safe, can you point me to the class that you say must be the > culprit. Maybe I can do a code review if I can figure out what this caching > does. The culprit would probably be AbstractCachingDMVMProvider, but it could also be other classes involved in the caching logic. It's a really complicated piece of code, so you'd have to invest some time to understand it. (In reply to comment #17) > I'll be honest and admit I didn't understand most of this propertyProvider > stuff. So, I had a look at the code I was familiar with. Everything looks ok > from that end. That's what I was hoping for. > Some very minor comments on the new IDsfService.isActive() > method: > > - For Bug 256798 "[services] Services can be called after they are shutdown", > should we now have anyone using a service, call the new isActive() method, to > make sure the service has not been shutdown? Instead of simply checking for > null, which is not enough. I wasn't specifically trying to find another way around this issue, but I was just trying to simplify some logic in the UI code. It gave me an idea for dealing with that bug, so I wrote up a quick patch for it. > - AbstractMIControl.isActive() now overrides IDsfService.isActive(). It should > either be removed, or have the @Overrides annotation. > > - ICommandControlService can have its isActive() removed since it is now in > IDsfService. > > That's all I got. I didn't realize I was stepping on so many toes with this method. Thanks for pointing it out. I will rename it to IDsfService.isRegistered() instead.
Created attachment 128650 [details] Patch renaming IDsfService.isActive() to IDsfService.isRegistered() I committed this change to get rid of conflicts with other isActive() methods that Marc pointed out.
+1 for me. Asside from the issue I noted below this seems to be pretty good. That issue needs to be addressed. I will try and run it down myself after updating with the latest stuff. Given the newness of the code, my success will be questionable. Randy
(In reply to comment #19) > Created an attachment (id=128650) [details] > Patch renaming IDsfService.isActive() to IDsfService.isRegistered() > > I committed this change to get rid of conflicts with other isActive() methods > that Marc pointed out. I wonder if you didn't have the right idea with the isActive() method. If we equate registration with the service being active (which I think we can, since unregister is called by shutdown()), I think isActive() is more descriptive than isRegistered(). What I mean is that I can imagine using isActive() to know if a service should be used; on the other hand, I don't quite see why I would want to know if a service is registered. If I understand correctly, with isActive() (which is not used anywhere yet), I believe a view could fetch a service once and check isActive() when re-using it, instead of fetching it multiple times. Right? So, why not keep isActive() in IDsfService, and have a services (e.g., RunControl) override it if that service has a different implementation?
Created attachment 128731 [details] Need an initial VM state reason now that properties are generically gathered Need an initial VM state reason now that properties are generically gathered. The model asks for a reason name genericlly with PDAVMVirtualMachine. Originally the code set the VM reason to "null" and in the past it was always filled in before referenced. With the PROPERTY change work it now is accessed before it is set.
(In reply to comment #21) > I wonder if you didn't have the right idea with the isActive() method. If we > equate registration with the service being active (which I think we can, since > unregister is called by shutdown()), I think isActive() is more descriptive > than isRegistered(). What I mean is that I can imagine using isActive() to > know if a service should be used; on the other hand, I don't quite see why I > would want to know if a service is registered. > > If I understand correctly, with isActive() (which is not used anywhere yet), I > believe a view could fetch a service once and check isActive() when re-using > it, instead of fetching it multiple times. Right? > > So, why not keep isActive() in IDsfService, and have a services (e.g., > RunControl) override it if that service has a different implementation? It is kind of funny that we're talking about methods that aren't used anywhere yet :-) I created the IDsfService.isActive() because I was using it in the fix for this bug, but I later refactored it in a way that I didn't need it anymore. Still I think the feature is useful as an alternative to going through the OSGi bundle context messy API. I renamed it because I was afraid of the conflict with AbstractMIControl.isActive(), which is not the same as the IDsfService.isActive(). The MI control talks about when the service is processing commands. It could theoretically stop processing commands and still be registered for a while after that. I thought the isRegistered() is more accurate description, because that's how it's implemented in AbstractDsfServcie. The way I intend this to be used is that it could allow clients to call isRegistered() if they save the reference to a service. So that they don't necessarily have to call service tracker every time, which is a bit wordy in some situations. Did I convince you or would you still like to change it back to isActive() :-)? As an alternative we could instead rename ICommandControlService.isActive() to ICommandControlService.isProcessingCommands().
(In reply to comment #23) > It is kind of funny that we're talking about methods that aren't used anywhere > yet :-) I created the IDsfService.isActive() because I was using it in the fix > for this bug, but I later refactored it in a way that I didn't need it anymore. I wondered about that :-) > Still I think the feature is useful as an alternative to going through the > OSGi bundle context messy API. Agreed > [...] > The way I intend this to be used is that it could allow > clients to call isRegistered() if they save the reference to a service. So > that they don't necessarily have to call service tracker every time, which is a > bit wordy in some situations. I like it. It is exactly how I imagined it too. > Did I convince you or would you still like to change it back to isActive() I'm convinced. Let's leave it as isRegistered(). Thanks.
