Community
Participate
Working Groups
The attached patch includes the following enhancements: 1) The "Renderings" header is removed. For new users, the term is more confusing than helpful. For experienced users, it does not serve a purpose. This frees up view real estate. 2) The "Add Rendering" toolbar item is moved from the Rendering pane to the main view toolbar. 3) The "Remove Rendering" toolbar item is removed. 4) The Rendering tabs now use CTabItem/CTabFolder instead of TabItem/TabFolder. CTabFolder allows for the close "x" on the tabs (SWT.CLOSE). New renderings are added by the toolbar button and removed via the close tab. This emulated the tabbed web browser experience. A screenshot is also attached.
Created attachment 87007 [details] screenshot of proposed changes
Created attachment 87008 [details] patch
Nice! Besides the some spacing details this looks perfect. One question: Instead of the add rendering button on the toolbar did you consider having a special "add" tab to let the user select an additional rendering without opening a dialog?
"did you consider having a special 'add' tab" Yes, but I haven't seen the suggested UI model elsewhere in eclipse. If it might jeopardize the acceptance of this patch for 3.4, I'd prefer to start the discussion under a separate bugzilla. I think the 'add' tab versus a toolbar item is really a personal preference. Other opinions? "to let the user select an additional rendering without opening a dialog? " With this patch and 214956, the dialog path is avoidable: connect to target, open memory view, type an address in address bar, press enter, click add rendering to create an additional tab.
Thanks again for the patch. I will review and consider this for 3.4.
Created attachment 87120 [details] updated patch: New Rendering toolbar action creates a CreateRendering, eliminating the need for a rendering selection dialog.
Created attachment 87129 [details] updated patch 3 The "Add Rendering" toolbar item does not work for the second pane in split pane mode. Despite what I wrote in comment #4, I now find the always present "add rendering" tab to provide the most straight forward experience. The attached patch (updated patch 3) includes the following enhancements: 1) The "Renderings" header is removed. For new users, the term is more confusing than helpful. For experienced users, it does not serve a purpose. This frees up view real estate. 2) The "Add Rendering" toolbar item is removed. 3) The "Remove Rendering" toolbar item is removed. 4) The Rendering tabs now use CTabItem/CTabFolder instead of TabItem/TabFolder. CTabFolder allows for the close "x" on the tabs (SWT.CLOSE). New renderings are added by the toolbar button and removed via the close tab. This emulated the tabbed web browser experience. 5) The CreateRendering rendering always exists as the right most tab. Adding a new rendering is as simple as selecting the "New..." tab and clicking the desired rendering type. A new screenshot is attached.
Created attachment 87130 [details] screenshot of proposed changes
Created attachment 87809 [details] patch updated against HEAD
Hi Ted, I really like this idea too :) This has been one of the usability problems that we have been trying to address. The extra add and remove rendering actions were confusing. Here are some feedback to the patch: 1) Using CTabFolder is ok, but it does not look very pretty on windows. I have added code to experiment with customizing the look of the tab folder. I simply copied the code from the launch config dialog. You can take a look and see if you are ok with it. (Will attach updated patch.) 2) Having a "Add..." tab at the end is also a good idea. However, when there are many tabs in the pane, the user may not be able to see the "Add..." tab. In that case, the user is required to navigate to the last tab to add a new rendering, which can be annoying. I think we need a shortcut key and global context menu action to bring the "Add Rendering" dialog back. The user can choose to use that tab, or use the action. (e.g. CTRL-T like in firefox :) 3) This patch also address an accessibility issue. We are required to allow the user to invoke all action without using the mouse. The "Remove Rendering Action" is replaced by the "Close" button on the tab. However, I can't seem to be able to tab to that button or invoke it from the keyboard or with any shortcut key. I think we need another global context menu action to remove the rendering. I also thinking having a shortcut key to remove the rendering maybe a good idea. Thanks... patch to follow. Samantha
Created attachment 88594 [details] patch to debug.ui
Created attachment 88595 [details] screen cap
Just to clarify, my updated patch only included a fix to make the tabs look better. It does not address (2) and (3). In addition, I also have the following feedback... :) (4) My sample debugger has default renderings set up. After adding a memory monitor, it adds a HEX rendering to the left, and integer rendering to the right. After the renderings are added, I clicked on the rendering to the left. However, the synchronization feature seems to be broken. The renderings do not move together anymore when the user navigate inside the rendering. This seems to be a timing thing. (5) Sometimes, the tab label does not show the address of the memory monitor. (I reproduced it by adding a rendering from the "Create Rendering" tab. (6) The label on the "Create Rendering" does not show which monitor the renderings are going to be created for anymore. The text widget on the rendering show "Memory Monitor: Add...". This could get quite confusing if the Memory Monitors pane is hidden. (7) I also think it's a good idea to add an icon to the "Add..." tab. This will make the tab stands out and that the user does not have to hunt for it.
Almost done... :) forgot about one last thing: Is it possible to not specify SWT.CLOSE for the "Add..." tab? Then we don't have to worry about user closing it and then recreating it. The current behavior is that when it is closed, the screen flashes and a new one is created.
Created attachment 90147 [details] 215432-memoryview5.txt comment #10 - 1) Looks good. Thanks for the help. comment #10 - 2) CTRL-N (ActionFactory.NEW) takes the user to the CreateRendering tab. comment #10 - 3) CTRL-W (ActionFactory.CLOSE) closes the active rendering. comment #13 - 4) I'll investigate. comment #13 - 5) I haven't been able to reproduce. Do you still see this behavior? comment #13 - 6) The address is now included in the label text. comment #13 - 7) Icon added. comment #14 ) We no longer specify SWT.CLOSE for the "New" CreateRendering tab.
Created attachment 90152 [details] 215432-memoryview6.txt
Created attachment 90700 [details] updated patch to debug.ui Thanks again Ted, I need more time to review the patch. However, I have made the following fixes: * I have fixed up the tab label and the "memory block" label in CreateRendering. * I have fixed the problem where the renderings are missing "becomesVisible" and "becomesHidden" notification. This is because ViewTabEnablementManager is not migrated to use CTabFolder and CTabItem. I have noticed a problem where I will have two CreateRendering created. I will attach screen capture. Step to reproduce, but cannot be reproduced consistently: 1> start debug session 2> create a memory monitor that has multiple default renderings I will get two "CreateRendering", one at the beginning and one at the "end". This is timing problem, but I haven't had time to track down what's going on.
Created attachment 90701 [details] screen cap with 2 CreateRendering
I also removed the "icon" constant to IDebugUIConstant. I reused the icon from "Add Rendering" action. I do not think the icon constant should be made API. In addition, I still have concern that the user will have trouble discovering the "Create Rendering" tab. In the case when I have a long list of renderings created by default, the "CreateRendering" will be pushed to the far end. The user may not be able to discover this extra tab and will end up having trouble creating new renderings. To solve, I still think that we need a "Add Rendering" context menu item from the rendering. What do you think?
Weird... as I am playing with this, I noticed that on my view pane on the left, I see no "Add Rendering" or "Remove Rendering" context menu action. On the view pane on the right, I see the actions. Invoking "Add Rendering", I get an extra "Create Rendering" tab with the "CLOSE" button. Adding a rendering there, will create a new rendering, however the "CreateRendering" tab is still there. This can pollute the view pane with many "CreateRendering" tabs.
Created attachment 91303 [details] latest patch for review
Ted, Thanks again. I tried out the new patch. I have run into problems that if I delete a memory block and create another one, the container is not presented with a "New Renderings..." tab. The user is left with no way of creating any rendering. There are also cases where I get a new "CreateRendering" created every time I swtich memory block. I believe the management of the "CreateRendering" neeed to be cleaned up because of this feature. It was originally designed such that if there is no rendering, the container will create a "CreateRendering" tab. If a rendering is added, the the container will remove this tab. I do not think the patches have properly cleaned up this algorithm. Ideally, we should create a "CreateRendering" whenever a container is created. We should remove all code that adds/removes the "CreateRendering" when a new rendering is added. I saw that you in RenderingViewPane.addRendering, you are allowing the "CreateRendering" to be added, but you handle it differently. You simply bring the existing "CreateRendering" to the top. I think this breaks the IMemoryRenderingContainer.addMemoryRendering(...) API contract. The container is supposed to simply add the rendering and create the control. The container should not distinguish whether it is a "CreateRendering" or not. A different approach is to add an method in RenderingViewPane to allow callers to bring "CreateRendering" to the top. Secondly, the RenderingViewPane does not have a toolbar anymore. However, the MemoryView continues to create the toolbar manager. You have only removed the creation of the actual control. To properly remove the toolbar, I believe the MemoryView#createRenderingViewPane method needs to be updated. We should not create the toolbar manager. We should stop calling RenderingViewPane.getActions to avoid creating actions unnecessarily. I am not very familiar with #setGlobalActionHandler. Your defined the "Add Rendering" and "Remove Rendering" shortcuts by modifying the global action handler. However, I do not see any code where the global action handler is set back to null. I have looked at how the Variables View work with "undo" or "copy", it sets the global action handler when the view has focus. It sets it back to null when the view loses focus. I believe we need to do the same to be safe. If you know more about this method, please let me know if this is not necessary. I am concerned that this is overriding global shortcuts although I cannot trigger the problem. This is what I have. I realize that we have gone through many iterations. If you think it will be useful, I can schedule a call do do a code review to speed up the process and also flush out all the remaining issues. Thanks... Samantha
Created attachment 92004 [details] latest patch for review Samantha, would you review the logic in RenderingViewPane.handleMemoryBlockSelection(...) after "// restore view tabs" and advise? toDisplay.getItemCount() now includes an instance of CreateRendering in the count.
Created attachment 92186 [details] Updated patch to debug.ui Fixed the following problems: * Restoring memory renderings does not work when the view is closed and reopened * NPE when closing the view - due to AddMemoryRenderingAction not created * Index out of bounds exception - due to getIndexOfCreateRenderingTab could return -1 * Updated copyrights * Removed fAddMemoryRenderingAction
Thanks again Ted. * I also changed the command handlers for "Add rendering" and "Remove rendering" actions I will submit the latest patch for IP approvals. Thanks... Samantha
Pending IP Approvals: http://dev.eclipse.org/ipzilla/show_bug.cgi?id=2181
IP Approved. Applied latest patch.
Verified.
