Community
Participate
Working Groups
Build Identifier: 3.7 M6 For variables view, expressions view, registers view, modules view and any derived views of VariablesView class, we need a way to override the find action. The find action is currently VirtualFindAction which is a good one for general cases; but there are specific cases that some debuggers need to provide their own find action to overcome missing functionality and performance issues, both of which are hard to get agreement by everyone across every programming language and every debuggers. See all comments at 340891 for one of the many reasons. Reproducible: Always
Created attachment 194218 [details] patch Please review and comment.
I'm sorry Winnie, but it's safe to say that it's too late to add new interface for 3.7. Also, it should be possible to have the context menu action invoke the same command as the key binding. Even then the change could interpreted as creating a form of a new API.
I also need to replace the find action with my own dialog for the Modules view. Also, this patch is something that can be use to replace any internal actions for views that are derived from Variables. Keybinding should work for the returning action by setting the action definition id as the VirtualFindAction. I understanding that it is too late to have new interface, but can this interface be made as internal/private for 3.7 release and make public for post indigo?
M6 is the last chance for 3.8 to get this in, Pawel can you review it before then?
Created attachment 212272 [details] Validated fix. I finally started looking at this bug today and I think it's a very compelling feature. It should allow debuggers to override not only the find action, but other standard actions such as double-click, etc. On the implementation side, I ran into some complications: the variables view resets the global action when the detail pane is activated and the contributed patch didn't account for that. In order to make it work I had to reorganize this remapping logic, which raises the risk that some bugs will creep in...
Hi Mike, If you have a sec, please take a look at this patch to see if you agree with how the API is being extended. I still need to do some more testing and cleanup, but I could commit it by weekend.
(In reply to comment #5) > Created attachment 212272 [details] > Validated fix. > I finally started looking at this bug today and I think it's a very compelling > feature. It should allow debuggers to override not only the find action, but > other standard actions such as double-click, etc. > On the implementation side, I ran into some complications: the variables view > resets the global action when the detail pane is activated and the contributed > patch didn't account for that. In order to make it work I had to reorganize > this remapping logic, which raises the risk that some bugs will creep in... Pawel, I don't see how your code use IViewActionProvider.getAction while I see the code of using IViewActionProvider.getUpdatables. Is it just something you forgot to put in your patch?
Created attachment 212309 [details] Patch with getAction(). (In reply to comment #7) > Pawel, > I don't see how your code use IViewActionProvider.getAction while I see the > code of using IViewActionProvider.getUpdatables. Is it just something you > forgot to put in your patch? Indeed. I went back and forth between overriding getAction() and setAction(). Here's the one with getAction().
As I'm testing, I'm seeing more problems with the patch. Will post an update in a bit.
-1 from me for this. If I understand what is wanted - a way to inject an action to override a default action (in this case specifically the find action) - then I am not sure this patch accomplishes that. 1. the getAction method is not used at all - I would assume it would be called during the view init to ensure the actions are overridden correctly 2. the new getUpdatables method makes no sense to me. could we not just add the custom actions to the global updatables map so they will be queried for their update status? why do we need more process for that? Or is that what / where that method is supposed to be used? 3. why the visibility change in VirtualFindAction? 4. the date + comments need clean up in IViewActionProvider
Created attachment 212325 [details] Updated and tested patch. (In reply to comment #10) > -1 from me for this. > > If I understand what is wanted - a way to inject an action to override a > default action (in this case specifically the find action) - then I am not sure > this patch accomplishes that. Sorry, I rushed last night to get you to look at it because I wanted to know whether it was in the right direction. But I obviously should have waited till I got it cleaned up. This patch also includes an addition to the PDA example to use this API as a way of verifying it. I've arrived at this version after a morning of testing. It seems to work correctly though I was seeing one bug which I didn't definitively trace to any exception: the popup menu would stop being filled with the standard set of actions, instead it just had one element in it, and fillCotnextMenu was not being called. It was somewhat intermittent when I saw it and I can't reproduce it in this latest version of the patch but I'll keep trying. One curious thing I noticed in the process is that the Breakpoints view didn't properly re-update the redo/undo actions when detail pane was activated. It also didn't register a find action. If you have a chance please take a look again. The logic works for the attached PDA example, but it's grown to a fairly big change. > 1. the getAction method is not used at all - I would assume it would be called > during the view init to ensure the actions are overridden correctly This was a refactoring error. > 2. the new getUpdatables method makes no sense to me. could we not just add the > custom actions to the global updatables map so they will be queried for their > update status? why do we need more process for that? Or is that what / where > that method is supposed to be used? I was concerned that the overriden action would not be updated correctly. With some refactoring I ended up removing this part. > 3. why the visibility change in VirtualFindAction? This was to enable the test code in PDA. In this patch, I copied the action instead. > 4. the date + comments need clean up in IViewActionProvider Done.
Created attachment 212350 [details] Removed printfs
+1 the only remaining issues are cosmetic: 1. unused imports in examples.ui 2. unused local var in debug.ui 3. spelling mistake in IViewActionProvider
I cleaned up warning, pushed out the fix and merged to integration. Thank you for the quick review Mike. http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=c98ef3633d90d05ac5a82c928a9e20cd46435f3f and http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=64678065cd9fed43d2a1141912ece7672ca192c6
