Bugzilla – Full Text Bug Listing
| Summary: | [Registers View] Find dialog doesn't show registers under collapsed nodes | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Norman Yee <normankyee> | ||||||
| Component: | cdt-debug | Assignee: | cdt-debug-inbox <cdt-debug-inbox> | ||||||
| Status: | NEW --- | QA Contact: | Jonah Graham <jonah> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | cdtdoug, dorin.ciuca, pawel.1.piech, wlai | ||||||
| Version: | 7.0 | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 340710 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Norman Yee
Created attachment 192220 [details] Fix for bug34089. There is a known problem with this patch. In the init() method of RefreshRegistersViewActionDelegate, I replaced the Registers view's find action with my own. The problem is that the delegate's init() method must be called first before you bring up the Find dialog. I found a case where it isn't called in time: if you bring up Eclipse with the Registers view already open and press CTRL+F, you'll see the old Find dialog because init() hasn't been called yet. Then if you press CTRL+F a second time, the new Find dialog is shown. Anyone know of a way to fix this? (In reply to comment #1) This patch does not work. I tested it on two debuggers, one is dsf gdb and the other is a dsf-based debugger. The patch hangs the main thread inside org.eclipse.cdt.dsf.debug.internal.ui.viewmodel.actions.FindRegistersAction forever. Also, I don't understand the plugin.xml is changed that you replace the original Refresh action (RefreshActionDelegate) with the new refresh action RefreshRegistersViewActionDelegate, where you define your find registers action. I don't see the two actinos need to be related. Is it you are just experimenting to hook up your own find action to replace the original find action (VirtualFindAction). There must be a better way to do it, right? Doh! I forgot to test the patch against DSF gdb. I've fixed the hang and tested it against DSF gdb and the custom DSF debugger that we're developing. I'll attach the new patch. Thanks for catching the problem.
> Is it you are just experimenting to hook up your own find action to replace the original find action (VirtualFindAction). There must be a better way to do it, right?
Yes, that's correct. I modified plugin.xml to hook up my find action in RefreshRegistersViewActionDelegate.java.
I couldn't find a better way to replace the original find action. Ideally, I'd like to change RegistersView.java in org.eclipse.debug.ui so that it calls setAction() to replace the original find action. I can't do that in org.eclipse.debug.ui because my find action uses DSF so my changes have to be in the org.eclipse.cdt.dsf.ui plugin. I think the only place where I would have access to the RegistersView object would be the a register view's action delegate's init() function? Is there a better way to override the original find action?
Created attachment 192325 [details]
Fixes hang and and some minor problems.
Hi Norman, Have you considered the solution I suggested on the CDT dev list (http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg21480.html). This would be to extend the platform find dialog to perform a search on un-expanded elements. I believe it may end up a being a smaller patch and it would be generic to all virtual viewer based views. (In reply to comment #5) > Hi Norman, > Have you considered the solution I suggested on the CDT dev list > (http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg21480.html). This would be to > extend the platform find dialog to perform a search on un-expanded elements. I > believe it may end up a being a smaller patch and it would be generic to all > virtual viewer based views. Hi Pawel, I looked at BreakpointsView.expandAllElementsInViewer() and used code based on it to expand a collapsed node. How would it be used in a generic solution? Are you suggesting that all nodes in the registers view should be expanded first before bringing up the find dialog? (In reply to comment #6) > How would it be used in a generic solution? Are you suggesting that all nodes > in the registers view should be expanded first before bringing up the find > dialog? Not literally, only in the virtual viewer. I think as the virtual is auto-expanding and fetching data from the model, it can be a synchronous content provider for the find dialog. The tricky would be to make the fetching of data cancellable and throttled, so that too much data is not requested from the model at once. (In reply to comment #7) > (In reply to comment #6) > > How would it be used in a generic solution? Are you suggesting that all nodes > > in the registers view should be expanded first before bringing up the find > > dialog? > > Not literally, only in the virtual viewer. I think as the virtual is > auto-expanding and fetching data from the model, it can be a synchronous > content provider for the find dialog. The tricky would be to make the fetching > of data cancellable and throttled, so that too much data is not requested from > the model at once. Ok, I get it. Thanks for the explanation. Wouldn't that be slow though? Correct me if I'm wrong but when all the nodes in the virtual viewer are expanded and updated, wouldn't it have to read every register value and bit field value? In our case, our debug target has over a 1,000 registers and bit fields so there is a lot of data to read. If we have to communicate with the target over USB or the network, it would be even slower. In the patch I posted, it uses the registers service to get the group names, register names, and bit field names to display in the Find dialog, so it doesn't need to fetch the register/bit field values from the debug target. (In reply to comment #8) Good point. We don't want to fetch the values, so we should only retrieve the name column label to show in the find dialog. If the view model is coded correctly, it will not retrieve the values. The problem is that the name column ID is not in a public API so there's no way for the find dialog to assume what it is. However, I recently came across another use case where I need these APIs to be public (see bug 340710), and fixing that bug would enable this solution as well. That said, there's other challenges. In variables views, the trees may be infinitely deep if there's recursion in the model. The viewer should have a circuit breaker to stop expanding an element if that element is already in its tree path. Also the expand-all retrieval could be very CPU intensive for some models (e.g. java logical structures), so that's why there needs to be logic in place to throttle the requests to the model to avoid overwhelming the system. Even with all these challenges, you'd need to do relatively little coding (by mostly be re-using existing components) and you'd be providing a lot more value to all Eclipse debuggers, than the with the model-specific solution for the registers view... not to mention the use of knowledge of internal implementation of the find action which could change in future ;-) (In reply to comment #3) > I couldn't find a better way to replace the original find action. Ideally, I'd > like to change RegistersView.java in org.eclipse.debug.ui so that it calls > setAction() to replace the original find action. I can't do that in > org.eclipse.debug.ui because my find action uses DSF so my changes have to be > in the org.eclipse.cdt.dsf.ui plugin. I think the only place where I would > have access to the RegistersView object would be the a register view's action > delegate's init() function? Is there a better way to override the original > find action? Yes, by providing a handler within a context for the global “FindReplace” command Id. Something like this in your plugin.xml (could be org.eclipse.cdt.dsf.ui): <extension point="org.eclipse.ui.handlers"> <handler commandId="org.eclipse.ui.edit.findReplace" class="org.eclipse....FindRegisterHandler"> <activeWhen> <and> <with variable="activePartId"> <equals value="org.eclipse.debug.ui.RegisterView" /> </with> <test property="some property to make sure we are using dsf debugger"> </test> </and> </activeWhen> </handler> </extension> (In reply to comment #9) > That said, there's other challenges. In variables views, the trees may be > infinitely deep if there's recursion in the model. The viewer should have a > circuit breaker to stop expanding an element if that element is already in its > tree path. Also the expand-all retrieval could be very CPU intensive for some > models (e.g. java logical structures), so that's why there needs to be logic in > place to throttle the requests to the model to avoid overwhelming the system. How about huge arrays variables? How do you deal with them? I think that the code would get quite complex and error prone to cover all use cases for a generic solution, especially for variables view. On the other hand, a debugger knows its model and it has its model proxy to fire updates in a debug view. It can override the handler for “Find” command and it can build a model delta according to what user selects: parent (EXPAND) -> parent (EXPAND) -> leaf (SELECT) (In reply to comment #10) > Yes, by providing a handler within a context for the global “FindReplace” > command Id. > Something like this in your plugin.xml (could be org.eclipse.cdt.dsf.ui): The variables view registers the find action with the action bars programmatically. I don't know if the command framework will ever prioritize the contributed handler to be active. But it is worth a try. (In reply to comment #11) > How about huge arrays variables? How do you deal with them? I think that the > code would get quite complex and error prone to cover all use cases for a > generic solution, especially for variables view. I think it would only get as complex or error prone as the lazy loading viewer that we already have. The bigger concern is the amount of memory that loading all the model elements into a content provider would take. The better approach would be to write the output of a deep find into the search view. > On the other hand, a debugger knows its model and it has its model proxy to > fire updates in a debug view. It can override the handler for “Find” command > and it can build a model delta according to what user selects: parent (EXPAND) > -> parent (EXPAND) -> leaf (SELECT) Sure it's easier at first, but you'll be creating a model-specific piece of code that will have to be updated over time with changes to the model. In the aggregate it is much more complex and expensive. Forgive me for being pushy ;-) I'm just looking at this with my platform hat on because there's been repeated requests for this feature from different sides. (In reply to comment #12) > The variables view registers the find action with the action bars > programmatically. I don't know if the command framework will ever prioritize > the contributed handler to be active. But it is worth a try. It does work, I tested it. The provided handler is called when pressing CTRL-F in registers view. However, there is indeed an entry in context menu “Find ...” that is hardcoded in org.eclipse.debug.internal.ui.views.variables.VariablesView.fillContextMenu(IMenuManager). But I guess this is a problem. It should use the global action handler as CTRL-F does. > I think it would only get as complex or error prone as the lazy loading viewer > that we already have. I was referring to the special treatment that should address corner cases when loading all model like infinitely deep trees (because recursion in the model), huge arrays etc. I guess the lazy loading viewer doesn’t deal with such use cases. > Forgive me for being pushy ;-) I'm just looking at this with my platform hat on > because there's been repeated requests for this feature from different sides. I agree that it would be nice to have a general solution for all debug views as you proposed. I just don’t see right now how all pieces would fit together in this general solution for variables view. (In reply to comment #13) > ... > It does work, I tested it. The provided handler is called when pressing CTRL-F > in registers view. However, there is indeed an entry in context menu “Find ...” > that is hardcoded in > org.eclipse.debug.internal.ui.views.variables.VariablesView.fillContextMenu(IMenuManager). > But I guess this is a problem. It should use the global action handler as > CTRL-F does. We discussed this in the platform debug meeting today (http://wiki.eclipse.org/Debug/Phone_7-Apr-2011), and we concluded that there should be a separate bug for enabling the overriding of the find handler for the variables view. Please open this bug as a client. We'll discuss there whether this is something we can address in Indigo or in general. Thanks, Pawel (In reply to comment #14) > (In reply to comment #13) > > ... > > It does work, I tested it. The provided handler is called when pressing CTRL-F > > in registers view. However, there is indeed an entry in context menu “Find ...” > > that is hardcoded in > > org.eclipse.debug.internal.ui.views.variables.VariablesView.fillContextMenu(IMenuManager). > > But I guess this is a problem. It should use the global action handler as > > CTRL-F does. > We discussed this in the platform debug meeting today > (http://wiki.eclipse.org/Debug/Phone_7-Apr-2011), and we concluded that there > should be a separate bug for enabling the overriding of the find handler for > the variables view. Please open this bug as a client. We'll discuss there > whether this is something we can address in Indigo or in general. > Thanks, > Pawel Did someone open a bug about overriding the find handler? (In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > ... > > > It does work, I tested it. The provided handler is called when pressing CTRL-F > > > in registers view. However, there is indeed an entry in context menu “Find ...” > > > that is hardcoded in > > > org.eclipse.debug.internal.ui.views.variables.VariablesView.fillContextMenu(IMenuManager). > > > But I guess this is a problem. It should use the global action handler as > > > CTRL-F does. > > We discussed this in the platform debug meeting today > > (http://wiki.eclipse.org/Debug/Phone_7-Apr-2011), and we concluded that there > > should be a separate bug for enabling the overriding of the find handler for > > the variables view. Please open this bug as a client. We'll discuss there > > whether this is something we can address in Indigo or in general. > > Thanks, > > Pawel > Did someone open a bug about overriding the find handler? I just filed a bug 344023 to track this. > I just filed a bug 344023 to track this.
I just submit a patch of 344023 that allows us to override an action.
|