Community
Participate
Working Groups
When using update policies it's often convenient to refresh all debugger views with a single click. To implement this feature this bug covers the following: 1) Create a new action set for Debug update policies There is a growing number of commands and actions related to update policies, which are increasingly cluttering the debug UI. Frankly, only a small percentage of DSF-based debugger users will actually ever care about update policy, so exposing them to all these modes and actions by default is only confusing. A new action set can be used to hide all this update mode functionality by default and let users and commercial debuggers make them visible them as needed. Therefore, this action set should show/hide not only the new refresh all action, but also the existing view sub-menus and toolbar buttons related to update modes (i.e. Update Policies menu, Update Scopes menu, Refresh action, etc.). 2) Create a new *retargetable* Refresh All action. This action is added to the main menu and toolbar. Since it is hidden by default, it should not create real estate issues on the toolbar. This action needs to be regrettable, which means that specific DSF debugger implementations need to supply their own adapter to implement this command. The reason for this is that besides the standard view model views, there are potentially many other views that may need to be refreshed (e.g. memory view, disassembly, etc.). 3) Modify the current Refresh action in debugger views to re-use the standard refresh command declared by the Workbench. This is the Refresh command found in the file menu, which can be invoked through the F5 key binding. Unfortunately, this default key binding is obscured by the Step Into command while debugging, but it at least allows the user to reconfigure that if desired.
Created attachment 115594 [details] Patch containing the feature. This patch implements the new "Refresh All" feature as described above. I've spend some time investigating the different menus/commands options for implementing this and I think using the actionSets extension point, along with creating a custom retargetable action class is still the best choice. I also struggled with the best way to hide/show the update policy actions in the views. With this patch, these actions will show only when the action set is enabled AND a debug target supporting update policies is enabled. This is probably an overkill, but on the other hand debugger UI is getting so cluttered that I thought it's better to err on side of fewer icons.
The suggested patch contains a small API change. Since we are past the API freeze, I will send a petition to the dev list before checking it in. The API change consists of the following new interface: /** * Extension to the IVMAdapter interface which allows access to the arra of active * providers. * * @since 1.1 */ public interface IVMAdapterExtension extends IVMAdapter { /** * Retrieves the currently active VM providers in this adapter. * * @return array of VM providers */ public IVMProvider[] getActiveProviders(); } Strictly speaking this new interface is not absolutely necessary for the new functionality. However, the method getActiveProviders() replaces the AbstractVMAdapter.getProvidersIterable() protected method which was added in 1.1 as well, but which is not completely safe in this thread-safe interface. So I think it also has this positive side effect.
(In reply to comment #2) > However, the method getActiveProviders() replaces the > AbstractVMAdapter.getProvidersIterable() protected method which was added in > 1.1 as well Technically, your new API baseline should now be 1.1 M3 instead of 1.0. (there are actually 4 errors about incrementing the minor version of some plugins that you'lss see...) If you set that baseline, you will notice that this change is actually a breaking change, since AbstractVMAdapter.getProvidersIterable() is gone from 1.1M3. To be really proper, you should keep AbstractVMAdapter.getProvidersIterable() (maybe deprecate it?) That being said, I'm sure no one will notice this particular API change, but then again, where do we draw the line? (and the API tooling will notice :-)) You have my +1 about adding the new interface. The breaking part though, I worry about opening that door :-)
(In reply to comment #3) > (In reply to comment #2) > > However, the method getActiveProviders() replaces the > > AbstractVMAdapter.getProvidersIterable() protected method which was added in > > 1.1 as well > > Technically, your new API baseline should now be 1.1 M3 instead of 1.0. (there > are actually 4 errors about incrementing the minor version of some plugins that > you'lss see...) This is an importaint point, but I actually don't believe it's a good idea to treat the milestone as a release. APIs and versions are set at release time, while the purpose of the API freeze is to avoid late breaking changes in dependent components (projects) in a given release cycle. If we took this logic to the extreme we may need base lines for M3, M4, RC1, etc...
+1 Randy
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > However, the method getActiveProviders() replaces the > > > AbstractVMAdapter.getProvidersIterable() protected method which was added in > > > 1.1 as well > > > > Technically, your new API baseline should now be 1.1 M3 instead of 1.0. (there > > are actually 4 errors about incrementing the minor version of some plugins that > > you'lss see...) > > This is an importaint point, but I actually don't believe it's a good idea to > treat the milestone as a release. APIs and versions are set at release time, > while the purpose of the API freeze is to avoid late breaking changes in > dependent components (projects) in a given release cycle. If we took this > logic to the extreme we may need base lines for M3, M4, RC1, etc... You are right. I just think it is nice to have the baseline set to the API freeze build so as to make sure we don't remove things that were added after 1.0, but that are now 'frozen'. I wonder if the API tooling allows for this.
Created attachment 115644 [details] Patch with remaining changes, I committed the part of the patch which didn't have any API changes, and this patch contains the remaining part, which needs the committer vote approval.
Created attachment 115647 [details] Updated patch with remaining changes. I accidentally left out a couple of files from the last patch.
(In reply to comment #8) > Created an attachment (id=115647) [details] > Updated patch with remaining changes. > > I accidentally left out a couple of files from the last patch. I'm still curious on what you think about *removing* a method after the API freeze.
(In reply to comment #9) > I'm still curious on what you think about *removing* a method after the API > freeze. > Removing or otherwise making any API change that's going to break API compatibility, after the API freeze, is definitely potentially more disruptive to the clients of that API. So this fact should be considered when deciding to make such a change. It's all a matter of judgmen, and as DSF gains more clients, these issues are going to become more serious. For now, I think it's great that we've created a culture where we understand and care about API compatibility and we follow processes that are reflective of that :-)
(In reply to comment #10) > For now, I think it's > great that we've created a culture where we understand and care about API > compatibility and we follow processes that are reflective of that :-) So you still want to remove AbstractVMAdapter.getProvidersIterable()?
(In reply to comment #11) > So you still want to remove AbstractVMAdapter.getProvidersIterable()? Yes, this method returns a reference to a collection to which access needs to be protected with a synchronized section. I think it's safer to have it return an array copy of the collection.
+1 for removing the method AbstractVMAdapter.getProvidersIterable() As the method has been added in 1.1, I think it is OK to remove it before it is being released. If we deprecate the method, we would release a new deprecated method. That does not make much sense either. BTW, using 1.1M3 as new API baseline is a good idea!
(In reply to comment #12) > (In reply to comment #11) > > So you still want to remove AbstractVMAdapter.getProvidersIterable()? > > Yes, this method returns a reference to a collection to which access needs to > be protected with a synchronized section. I think it's safer to have it return > an array copy of the collection. +1 for me. Some comments on the patch: 1- GdbAdaptorFactory.SessionAdapterSet.dispose() was not updated for this RefreshAll 2- Copyright header may need some changes for (we don't usually keep names): RetargetDebugContextAction.java RetargetAction.java 3- @since 3.0 label is wrong for RetargetAction.java 4- I didn't have time to try the patch, so I'm not sure if this is relevant. I just wanted to point out that Refreshing the views is also useful for non-stop mode where data can change while the user is looking at it. This may give another use case for the RefreshAll which maybe should not be only linked to UpdateModes (unless I misunderstood...)
(In reply to comment #14) > 1- GdbAdaptorFactory.SessionAdapterSet.dispose() was not updated for this > RefreshAll This is not by chance. DefaultRefreshAllTarget just doesn't have a dispose() method. > 2- Copyright header may need some changes ... Thanks for pointing this out. I cleaned it up (actually RetargetAction got into the patch by mistake, it should have been removed). > 4- ... This may give another use case for the RefreshAll ... True enough. Our users certainly use the refresh action to make sure that the data in the views is current (regardless of the update mode). I do think we need to categorize these actions using the new action set to avoid clutter, so that if user wants to see them he can enable them. BTW, the regular refresh action is also linked to the standard Refresh command (File menu -> Refresh). So it's accessible even when the view menu item isn't there.
We've had the vote open almost the whole week with no objections, so I'm going to go ahead and commit it. Randy please review. P.S. Ironically this is going to break Wind River's refresh all implementation which uses the now removed getProvidersIterable(). So we'll need to change this to the new getProviders() call once we pick up a new DD 1.1 build.
GdbAdapterFactory.java was not committed as part of the last commit... > > 1- GdbAdaptorFactory.SessionAdapterSet.dispose() was not updated for this > > RefreshAll > This is not by chance. DefaultRefreshAllTarget just doesn't have a dispose() > method. The call to session.unregisterModelAdapter(IRefreshAllTarget.class) should be done, no? (simply based on the fact that we do it for all the other ones, except for ILaunch and IDebugModelProvider.) > BTW, the regular refresh action is also linked to the standard Refresh command > (File menu -> Refresh). So it's accessible even when the view menu item isn't > there. These are grayed out for me... This seems to be an old problem because if I run with M3, I still have the refresh buttons (e.g., in the variable view) and can use them, but the File->Refresh is grayed out. After applying GdbAdapterFactory, I'm trying to test the changes. The refresh buttons are gone from the views along with the Update Policy menus. But can I enable them back on Linux? Or are update policies gonna be kept hidden for Linux?
(In reply to comment #17) > The call to session.unregisterModelAdapter(IRefreshAllTarget.class) should be > done, no? (simply based on the fact that we do it for all the other ones, > except for ILaunch and IDebugModelProvider.) Yes, you are correct. I missed this detail. > These are grayed out for me... This seems to be an old problem because if I run > with M3, I still have the refresh buttons (e.g., in the variable view) and can > use them, but the File->Refresh is grayed out. I somehow missed the GdbAdapterFactory part of the patch in the last commit. I applied it now so the refresh all action should now work with GDB. > After applying GdbAdapterFactory, I'm trying to test the changes. The refresh > buttons are gone from the views along with the Update Policy menus. But can I > enable them back on Linux? Or are update policies gonna be kept hidden for > Linux? You need to enable the "Debug Update Modes" action set (through the Windows->Customize Perspective action, Commands tab). I think it's better to hide this by default, but if the consensus is to have it enabled by default we can change it. Also I changed the refresh toolbar icon in the views to appear only when a DSF debugger connection is active, to be consistent with the update modes menus. But I'm not entirely sure if this is a good idea. For one, it's somewhat of an overkill since the action set can now be used to hide this, but also because there's an annoying bug in the command framework, which causes the actions to be hidden if the given view doesn't have the focus.
(In reply to comment #18) > You need to enable the "Debug Update Modes" action set (through the > Windows->Customize Perspective action, Commands tab). Thanks, I got it to work. > I think it's better to > hide this by default, but if the consensus is to have it enabled by default we > can change it. My vote is to have the update modes hidden by default but to have the refresh buttons shown by default... Is that possible? > Also I changed the refresh toolbar icon in the views to appear only when a DSF > debugger connection is active, ... This started off by being a little annoying, but after playing with it, it became an actual problem :-) If you select the editor, and then press the Terminate button in the debug view, or the Resume button, or any other button; in my case, the button jumps right out from under my mouse and does not get clicked! Some other comments: 1- In my case, File->Refresh is grayed out unless I have "Debug Update Modes" on. 2- If I have "Debug Update Modes" off, then launch a DSF debug session, then turn on "Debug Update Modes", the RefreshAll button remains grayed out, although the individual refresh buttons are available. 3- When turning off "Debug Update Modes", I suggest that all update modes go back to their defaults. I had set Manual mode for the variable view and then turned off "Debug Update Modes", but then nothing got updated in the variables view and I couldn't change it or refresh. 4- I don't know what "update scopes" are, but I saw them once in the view menu, but now I never see them, even when "Debug Update Modes" is on.
(In reply to comment #19) > My vote is to have the update modes hidden by default but to have the refresh > buttons shown by default... Is that possible? I suppose it is by creating another action set. But that would probably be too granular. The other alternative is to have the refresh action always there, but that goes against the goal of reducing the overall clutter, especially for non-DSF debuggers. > > Also I changed the refresh toolbar icon in the views to appear only when a DSF > > debugger connection is active, ... > > This started off by being a little annoying, but after playing with it, it > became an actual problem :-) If you select the editor, and then press the > Terminate button in the debug view, or the Resume button, or any other button; > in my case, the button jumps right out from under my mouse and does not get > clicked! Yean, I'm discovering too that adjusting the visible toolbar buttons dynamically is a bad idea :-( > Some other comments: > 1- In my case, File->Refresh is grayed out unless I have "Debug Update Modes" > on. I'll file a bug. > 2- If I have "Debug Update Modes" off, then launch a DSF debug session, then > turn on "Debug Update Modes", the RefreshAll button remains grayed out, > although the individual refresh buttons are available. This is also a bug. > 3- When turning off "Debug Update Modes", I suggest that all update modes go > back to their defaults. I had set Manual mode for the variable view and then > turned off "Debug Update Modes", but then nothing got updated in the variables > view and I couldn't change it or refresh. This may be rather tricky to fix. I'll file a bug and leave it on back burner. > 4- I don't know what "update scopes" are, but I saw them once in the view menu, > but now I never see them, even when "Debug Update Modes" is on. After some soul-searching we concluded that the value of update scopes is very questionable (and a source of many bugs). So we decided to hide them for now.
One last thing I noticed: - when selecting the gdb process or inferior process in the debug view of a DSF launch, the RefreshAll is grayed-out but the refresh buttons are not. (In CDI everything is properly grayed out.)
Looks like it is there now. Randy
DD 1.1 reelased!
