Community
Participate
Working Groups
This request includes a patch with the following Memory View workflow enhancements: 1) Memory Monitors Pane is not visible by default. 2) Go To Address control is moved from the renderings to the Memory View, between the toolbar and the content area (without breaking support for existing renderings). 3) Go To Address: "Cancel" button is removed, since it is no longer necessary. 4) Go To Address: "Ok" button is renamed to "Go" 5) Toolbar item is added to toggle the visibility of the Go To Address control, with a default state of on. 6) Go To Address: Enter Key has the same effect as clicking the "Go" button. 7) Go To Address will: a) Adjust the base address of the active rendering's memory block, if the block supportBaseAddressModification() equals true. If not: b) Reposition the active rendering, if it implements IRepositionableMemoryRendering. If not: c) Fall back case: Creates a new memory block at the specified address. These minor enhancements improve the experience for new and experienced users. New users are not burdened with learning the meaning of memory blocks, renderings and their association. All users benefit from fewer necessary clicks. This patch attempts to address the most common use case: take me to <address>.
Created attachment 86606 [details] Memory View workflow enhancements patch.
Created attachment 86609 [details] togglegotoaddresspane_co.gif
Created attachment 86610 [details] elcl16/togglegotoaddresspane_co.gif
Hi Ted, I like the "go to address" idea. In general there seems to be too many ways to do a couple of things in the memory view and the toolbar seems to cut through that clutter a bit. A few observations: 1) The toolbar takes a lot of screen space in a view that already has a lot of menus and buttons. Some ideas on how to address this: - use less padding around the go to address toolbar - When the "go to address" toolbar is present, do not show the "Monitors" and "Renderings" column headers. This way the go to address would replace the buttons used to create the monitors and renderings. - Instead of a permanent tool-bar, use a minimal popup window to just allow the user to enter the text. This kind of popup is used in the Java package explorer window when you press Ctrl-F 2) Every time I pressed "go" a new monitor was created. I think it would have made sense to re-use monitors in some cases. 3) Selecting a rendering after selecting "go" seems like an unnecessary step. It would be nice if a default rendering was used. Or if the selected rendering was remembered from the last time. 4) I got the exception below after trying to type in an expression in the go to field. Hope this helps. -Pawel java.lang.NumberFormatException: For input string: "cptr" at java.lang.NumberFormatException.forInputString(NumberFormatException.java:48) at java.lang.Integer.parseInt(Integer.java:456) at java.math.BigInteger.<init>(BigInteger.java:314) at org.eclipse.debug.internal.ui.views.memory.MemoryView$1.run(MemoryView.java:346) at org.eclipse.debug.internal.ui.views.memory.MemoryView$3.keyPressed(MemoryView.java:476) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:154) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1113) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1137) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1122) ...
Created attachment 86627 [details] Screenshot of memory view with the "go to" toolbar. I attached a screenshot of the view for reference.
"1) The toolbar takes a lot of screen space in a view that already has a lot of menus and buttons. Some ideas on how to address this: - use less padding around the go to address toolbar" I also considered eliminating padding and the pane headers. I'd be helpful to hear some feedback from the platform team in regards to what would be acceptable. "2) Every time I pressed "go" a new monitor was created." This is because DSF's MemoryBlock returns false from supportBaseAddressModification() AND the rendering did not implement IRepositionableMemoryRendering. The fall back case is to create a new monitor. "3) Selecting a rendering after selecting "go" seems like an unnecessary step. It would be nice if a default rendering was used. Or if the selected rendering was remembered from the last time." Support already exists for registering default renderings. "4) I got the exception below after trying to type in an expression in the go to field." I'll address this with a new patch.
Created attachment 86631 [details] Memory View workflow enhancements patch #2
Thanks very much for the patch. I will review and consider this for 3.4.
See also https://bugs.eclipse.org/bugs/show_bug.cgi?id=215432
Hi Ted, Can you give me some background on the usability problems that you are trying to address with this enhancement? Are you trying to combine the "Add Memory Monitor" action with the "Go To Address" bar? Thanks... Samantha
Samantha, One of the most common use cases for memory views is "take me to <address>". This can be accomplished today, but the task involves several steps. By promoting the "Go To Address" bar to a more prominent location, navigation is greatly simplified: the user performs navigation by entering the address or expression and pressing enter, similar to the workflow of navigating to a URL in a browser. The "memory monitors" pane is useful as an advanced feature. It is analogous to bookmarks/favorites. However, it uses considerable real estate in a region typically limited in width. For the 80% case, this feature is probably not necessary, especially with the address bar present. I suspect you may be concerned with bullet 7 in comment #1. I am, too. The result of the user operation should be deterministic to the user, regardless of the underlying debug model support. I'd like for the address bar to provide navigation with the memory monitors concept only coming in to play when the user wishes to "bookmark" an address. Any thoughts on how to make this more palatable? ted
Created attachment 87804 [details] Memory View workflow enhancements patch #3 1) Memory Monitors Pane is not visible by default. 2) Go To Address control is moved from the renderings to the Memory View, between the toolbar and the content area (without breaking support for existing renderings). 3) Go To Address: "Cancel" button is removed, since it is no longer necessary. 4) Go To Address: "Ok" button is renamed to "Go" 5) Toolbar item is added to toggle the visibility of the Go To Address control, with a default state of on. 6) Go To Address: Enter Key has the same effect as clicking the "Go" button. 7) Go To Address will: a) create a new memory block if the view is unpopulated, otherwise b) attempt to reposition the active rendering using the IRepositionableMemoryRendering interface, otherwise c) remove the selected block and add a new block. B over C has the benefit of preserving rendering preference state when navigating between addresses. It is undesirable for navigation to reset rendering attributes, such as "cell size" or "endian".
Hi Ted, I think having a address bar at the top is a good idea. I have experimented having it in the Memory Monitors pane before, but decided to removed it. The main issue with having an address bar is that the control for the address bar needs to be customizable. The Memory View allows clients to retarget the Add Memory Monitor action. If the action is retargetted, then we give the target complete control over how a memory monitor can be added. This allows clients to customize the "Add Memory Monitor" dialog, and the clients can prompt their users for additional information. CDT uses this feature for their address space support. To get it to work, we will likely need new APIs to allow us to customize the "Go To Address" composite. Or if you can find a way that we can use existing APIs, that will have a better chance of getting this patch in. (e.g. can we reuse the detail APIs? Can we reuse the renderings APIs and make it a specialized rendering pane?) This patch also breaks support for plain IMemoryBlock. To add an IMemoryBlock, the user is required to enter an address and a length. Because the "Go To Address" composite is not customizable, we did not prompt user for the additional information. Finally, I do not think it is a good idea to simply reuse the "Go To Address composite". In addition to prompting for an expression, the composite also allows the user to "Go to Offset", or "Jump to x Number of memory units". These concepts are not valid when there is no rendering available. Therefore, they should be properly disabled. Thanks... Samantha
Created attachment 108160 [details] updated patch This patch should address all concerns expressed in comment #13. The address bar component is now specified by the IMemoryBlockRetrieval. The new API consists of only one interface with one method. The IMemoryRendering interface is reused for pluggable go to address components. Support for IMemoryBlock is not impacted.
Ted, has this defect become a dup of Bug 244822? Sam
No, this is an incremental enhancement in the direction of Bug 244822.
I reviewed the latest patch. Here's how my understanding of the design: A new view pane with a specialized id is created in the Memory View. This view pane is called the go to address view pane. By default, the view pane is visible. A new toolbar button is added to allow the user to control its visibility. The "Go To Address" view pane is a fake memory rendering pane. The idea here is that the current debug context provides an adapter to a new interface called "IMemoryGoToAddressProvider". The "memory go to address provider" provides an id of a rendering, which should be used as the widget in the "go to address" pane. A new "go to address" rendering is created whenever the user switches debug context and resulted in a different memory block retrieval. Comments: ========== * There seems to be no default implementation / adapter for the standard debug model. I am not able to see the go to address pane in action because my memory view sample did not implement IMemoryGoToAddress provider. As a result, that viewpane remains empty all the time. Even if I want to implement the provider, I am not sure what the go to address rendering id is. This implies that existing debugger can be broken if they did not implement this new interface. Existing debuggers will get this empty "go to address" pane at the top of the view. We need to design this such that existing debuggers are not broken by this new view pane, and default go to address rendering should be provided if none is specified. * Since the go to address pane is an UI component, I don't think defining a new interface in the core, and going through the adapter mechanism for getting the rendering id is the right approach. I suggest creating a new rendering type called "go to address" rendering. We will probably need two, one for supporting IMemoryBlock and another one supporting IMemoryBlockExtension. * We can then modify the renderingBindings extension point and introduce an optional "go to address id" field. If this field is specified in the extension, then we will create the rendering as specified, and populate the "go to addresss" pane with that rendering. If the "go to address id" is not specified in the renderingBinding extension, then we will use one of the default "go to address" rendering that we have created. Clients should only specify one "go to address" rendering id in their binding definition, similar to how primaryId is defined in the extension point. * Alternatively, you can extend IMemoryRenderingBindingProvider interface. That interface is responsible for providing a rendering types that are tied to a memory block. Perhaps we can define IMemoryRenderingBindingProvider2 as follows: IMemoryRenderingBindingProvider2: IMemoryRenderingType[] getRenderingTypes(IMemoryBlock memoryBlock, Object context); The "go to address" view pane, will try to determine the rendering types to use by passing in its pane id as the context. Implementers return the "go to address" rendering type. The view pane can then create the customized "go to address rendering" I think this approach is simpler... and is probably more flexible than modifying the extension point. * With one of these approaches in place, the Memory View can probably treat the GoToAdress pane as other view panes, and won't have to handle that view pane differently. We should be able to get to the view pane, show/hide it, just like other rendering view panes. * One thing that I am not sure about is how this would affect implementers who have provided a retargettable "add memory block" action. (e.g. CDT). Since they have retargetted the action, they probably have provided a customized dialog for adding a memory block. If that's true, this move will also force them to provide the new "go to address" rendering in their rendering binding extension. (We may want to consider tying this work to how we should control action visibility, and allow clients to not see this view pane if no definition is provided.)
Created attachment 123331 [details] updated patch for review
Hi Ted, I think the workflow with the latest patch is a bit odd for the platform. Here's what I am seeing: 1) I launched the sample debug adapter for testing the memory view 2) The memory view opens up, toggling the go to address bar action to show the "Go To Address" rendering pane. ===> After turning on the go to address bar, I am left with a big empty space at the top of the Memory view. Nothing shows up. ===> Question: The intent of the go to address bar is to remove the need to use the "Add Memory Monitor" action to add a memory block. Should something shows up in the go to address bar, when there is currently no memory block, such that the user can use the address bar for adding the memory block? 3) I can't seem to be able to find a way to resize the go to address bar. I always have this big gap in the memory view. 4) So, I add a new memory block, and still nothing shows up until I make a selection in the Monitors view. ===> I see the go to address bar, but the go to address bar only contains a big text field. There is no instructions on what the text field is for and what I am supposed to do with it. 5) I typed something in the text box, and clicked "Go" ===> Nothing happened. If I have no rendering in the rendering pane, I expected a new rendering to be created. If I have a rendering in the rendering pane, I expected the rendering to reposition. Can you show me how you got the got to address bar working in your scenario? Perhaps I am missing something?
I tried to re-organized the patch, such that we can use the go to address bar to add memory block. The trick to get this working, is that the go to address container must be able to show "renderings" of the IMemoryBlockRetrieval object. The current design of the memory rendering extension does not seem to fit very well. Here's how I expect the the go to address bar to work: 1) The go to address bar should show a customized control for any object that was last selected. 2) The customized control acts in the context of the selection, the selection is an IAdatable object. * If an IAdaptable object returns an IMemoryBlockRetrieval from an getAdapter() call, then we should use the retrieval object to create a new memory block. The assumption here is that the user has selected something in the debug view / variables view, and that they want a new memory block. * If an IAdaptable object returns a IMemoryRendering, then we know that the user is focused at looking at memory rendering. In this case, we should position the selected rendering to the new address. * The ambiguous case is what we should do if a memory block is selected. If a memory block is selected, we should try to find a rendering that is currently in the memory rendering container. If exist, we should position the rendering to the new location. If there is no rendering, then we should create a new memory block. (I am assuming that there is always going to be a rendering.)
Samantha, I'm working on cleaning up the layout of the address bar pane. Would you clarify, "The current design of the memory rendering extension does not seem to fit very well." Is this the IMemoryRendering, the extension point or something else? Should we attempt to retrofit in this patch?
I mea(In reply to comment #21) > Samantha, > > I'm working on cleaning up the layout of the address bar pane. cool thanks! > > Would you clarify, "The current design of the memory rendering extension does > not seem to fit very well." Is this the IMemoryRendering, the extension point > or something else? Should we attempt to retrofit in this patch? > What I meant here is the IMemoryRendering is targetted to display content of memory block, and memory block only. My original intention to re-use the memory rendering extension point, and IMemoryRendering does not fit very well, when we try to create a rendering for IMemoryBlockRetrieval. Ability to contribute a rendering for IMemoryBlockRetrieval is key to getting the go to address bar to work. Otherwise, you can never get the address bar to show up without a memory block. I think we need IMemoryRendering to be able to "take" any adaptable object as an input. Alternatively, we need a different extension point, like the details pane. I am not very eagar to create another extension point in order to solve the same problem as the details pane, or generating rendering for a memory block. It seems like we have repeatedly solved the same problem with different extension point. But I am not sure what to do with it at the moment.
I assume this is not being worked on...
+1 on assumption.
