Community
Participate
Working Groups
Context info can be found at https://bugs.eclipse.org/bugs/show_bug.cgi?id=344053 Now that we control the diagram context menu, we should add Print, Zoom, and Save As Image actions to it to make it complete.
Bulk deferral of diagram-related items to the 0.5 release.
Hi team, I am very interested in having this feature for my adopter product, thus I'd like to offer to implement this feature. Would the team be open to reviewing my patch submission?
To clarify, I'm interested in having this feature make it into 0.5, so I'll attach a patch shortly for consideration.
Created attachment 215832 [details] Patch v1 Version 1 of the patch. This uses action handlers for all actions. I did not attempt to wire these actions to their workbench retargetable actions. Also I added these actions to the diagram form page toolbar, as well as the context menu for consideration. Also I added the zoomIn/Out to the "Appearance" group. Not sure if that is the right approach. The SaveAsImage and Print actions I didn't specify a group so they show up at the bottom of the context menu.
Created attachment 215833 [details] Images needed for patch The images in this zip file I copied from the following locations: SaveAsImage.gif is just a renamed version of this file: org.eclipse.pde.runtime_3.4.201.v20110928-1516.jar/icons/obj16/save_image_as_obj.gif Print.gif is just a renamed version of this file:
Created attachment 215834 [details] Images needed for actions added in the patch. The images in this zip file I copied from the following locations: SaveAsImage.gif is just a renamed version of this file: org.eclipse.pde.runtime_3.4.201.v20110928-1516.jar/icons/obj16/save_image_as_obj.gif Print.gif is just a renamed version of this file: org.eclipse.ui.browser_3.3.101.v20111019-1723.jar/icons/clcl16/nav_print.gif ZoomIn.gif is just a renamed version of this file: org.eclipse.gef_3.7.2.v20111106-2020.jar/org/eclipse/gef/internal/icons/zoom_in.gif ZoomOut.gif is just a renamed version of this file: org.eclipse.gef_3.7.2.v20111106-2020.jar/org/eclipse/gef/internal/icons/zoom_out.gif
Created attachment 215835 [details] Screenshot of new actions in both context menu and diagram header toolbar
Created attachment 215836 [details] Patch v2 I found that after using the zoomIn/Out actions I wanted to "reset" the zoom to original. Also, after going up and down in zoom level, I found that sometimes I could never quite get back to the 100% level, it was just a bit "off" like 99.99% or something wierd. So I added another action for zoom "actual" or "100%". What I have seen other GEF editors do is offer a combo box for the zoom levels. I wasn't sure if the sapphire header toolbars could handle a "combo" so I just added a third zoom action button for now.
Created attachment 215838 [details] Patch v5 Images The images in this zip file I copied from the following locations: SaveAsImage.gif is just a renamed version of this file: org.eclipse.pde.runtime_3.4.201.v20110928-1516.jar/icons/obj16/save_image_as_obj.gif Print.gif is just a renamed version of this file: org.eclipse.ui.browser_3.3.101.v20111019-1723.jar/icons/clcl16/nav_print.gif ZoomIn.gif is just a renamed version of this file: org.eclipse.gef_3.7.2.v20111106-2020.jar/org/eclipse/gef/internal/icons/zoom_in.gif ZoomOut.gif is just a renamed version of this file: org.eclipse.gef_3.7.2.v20111106-2020.jar/org/eclipse/gef/internal/icons/zoom_out.gif ZoomActual.gif is a new file that I created myself using Gimp. It is just a reworked version of ZoomIn, no other source files were used in creating that image.
> To clarify, I'm interested in having this feature make it into 0.5, so I'll > attach a patch shortly for consideration. We have until May 23rd to review and merge this work. > What I have seen other GEF editors do is offer a combo box for the zoom levels. > I wasn't sure if the sapphire header toolbars could handle a "combo" so I just > added a third zoom action button for now. We don't have support for a combo like that. We could present it as a drop-down menu from a single button, but then we'd have to figure out a generic zoom icon to use. A plain magnifying class would likely be mistaken for search. Let's leave it like you have it right now... > I did not attempt to wire these actions to their workbench retargetable > actions. Let's go ahead and do that like we just did for select all and delete. > Also I added these actions to the diagram form page toolbar, as well as the > context menu for consideration. That should be fine. > Also I added the zoomIn/Out to the "Appearance" group. Not sure if that is the > right approach. Let's go ahead and create a separate zoom group. > The SaveAsImage and Print actions I didn't specify a group so they show up at > the bottom of the context menu. Let's create an "export" group for these. Other minor comments... 1. There is a string that needs to be externalized in SaveAsImageDiagramActionHandler. 2. I think it would be better to have separate action handler classes for various zoom actions instead of a parameterized one.
A few comments: 1. Since we don't support zoom levels using combo box, there is no visual indicator of the current zoom level. We should implement "Zoom Actual" as a toggle action so that at least user knows he/she is looking at the diagram at its actual size. 2. We can hook the "Print" action with Window's Print menu by modifying SapphireEditorActionBarContributor and SapphireDiagramEditor slightly. But to add zoom to the menu (zoom is not a global action), we need to extend SapphireEditorActionBarContributor class significantly. Once Greg's patch is submitted, you can pass me the bug to wire up the menus.
> 1. Since we don't support zoom levels using combo box, there is no visual > indicator of the current zoom level. We should implement "Zoom Actual" as a > toggle action so that at least user knows he/she is looking at the diagram at > its actual size. A toggle wouldn't work here. What would it mean to un-toggle it? But it would work to use enablement to signal the same. I think that would work well.
> But to add zoom to the menu (zoom is not a global action), we need to extend > SapphireEditorActionBarContributor class significantly. Once Greg's patch is > submitted, you can pass me the bug to wire up the menus. Is it important to do this? I'd like to see print in the menu wired up because it is there, so it should work. Is there a general expectation from other gef editors for zoom menu items in the eclipse menu?
> > Is it important to do this? I'd like to see print in the menu wired up because > it is there, so it should work. Is there a general expectation from other gef > editors for zoom menu items in the eclipse menu? Graphiti does add zoom to the eclipse menu. But I don't see it as a necessity since the diagram header has them, diagram's context menu has them as well. Add zoom to the eclipse menu is going to add clutter and noise to it. I agree with you that since print there is there so we should make it work.
Ok. Let's leave zoom out of the menu and just wire up print.
Created attachment 215874 [details] Patch v3
(In reply to comment #10) > > To clarify, I'm interested in having this feature make it into 0.5, so I'll > > attach a patch shortly for consideration. > > We have until May 23rd to review and merge this work. > > > What I have seen other GEF editors do is offer a combo box for the zoom > levels. > > I wasn't sure if the sapphire header toolbars could handle a "combo" so I just > > added a third zoom action button for now. > > We don't have support for a combo like that. We could present it as a drop-down > menu from a single button, but then we'd have to figure out a generic zoom icon > to use. A plain magnifying class would likely be mistaken for search. Let's > leave it like you have it right now... > > > I did not attempt to wire these actions to their workbench retargetable > > actions. > > Let's go ahead and do that like we just did for select all and delete. Patch v3 the workbench actions are still not wired up and I didn't do anything because Shenxue mentioned the work needed in SsapphireEditorActionBarContributor. > > > Also I added these actions to the diagram form page toolbar, as well as the > > context menu for consideration. > > That should be fine. > > > Also I added the zoomIn/Out to the "Appearance" group. Not sure if that is the > > right approach. > > Let's go ahead and create a separate zoom group. > > > The SaveAsImage and Print actions I didn't specify a group so they show up at > > the bottom of the context menu. > > Let's create an "export" group for these. > > Other minor comments... > > 1. There is a string that needs to be externalized in > SaveAsImageDiagramActionHandler. > > 2. I think it would be better to have separate action handler classes for > various zoom actions instead of a parameterized one. The rest of these items have been addressed in Patch v3.
Created attachment 215877 [details] Patch v4 In this patch I tried to address the enabled/disabled state update for the Zoom Actual to only be enabled if the zoom is != 1.0. This is working for the context menu, but for the diagram header toolbar, the action enablement doesn't seem to update visually.
Created attachment 215880 [details] Patch v4a Re-submitted patch v4. This one has an attempt to wire up the disable/enablement state for zoom actual. It is working ok for the context menu but I can't seem to get it to function as expected in the header toolbar. Will need the team to take another look at what I'm doing wrong. Also to add the zoomListener I had to do a little hack in SapphireDiagramEditor to call out to the zoomActualHandler to perform a "postInit()" I don't know how to get a sapphireDiagramEditor instance when the handler is init()'ed.
> This one has an attempt to wire up the disable/enablement state for zoom > actual. It is working ok for the context menu but I can't seem to get it to > function as expected in the header toolbar. Will need the team to take > another look at what I'm doing wrong. Shenxue will take a look. > Also to add the zoomListener I had to do a little hack in SapphireDiagramEditor > to call out to the zoomActualHandler to perform a "postInit()" I don't know how > to get a sapphireDiagramEditor instance when the handler is init()'ed. Ok... So action has access to part, but not the presentation. Well, technically, you get access to presentation during execution, but you don't have it during init. In any case, where possible, we try to write actions in a way that is presentation agnostic... So what does that mean? Zoom level is a state. This state can be maintained by the editor page part. Then actions can call part.setZoomLevel() and synchronize their state (such as enablement) on zoom level change events. On the other side, the presentation (SapphireDiagramEditor) can listen on zoom level changes to coordinate with GEF zoom manager. So let's do this... 1. Add setZoomLevel()/getZoomLevel() methods to SapphireDiagramEditorPagePart. Probably use an integer. Similar to setGridVisible()/isGridVisible(). Just instead of using the deprecated listener API, it should use the new (o.e.s.Listener) system. Let's define ZoomLevelEvent for that. The rest should be straightforward use of attach/detach/broadcast API. 2. Move zoom action to the main UI plugin and have them use the new zoom level API. 3. Make SapphireDiagramEditor listener on ZoomLevelEvent and coordinate with GEF zoom manager. Let me know if anything above is unclear.
Created attachment 215891 [details] Patch v5
Submitted Patch v5 > > So let's do this... > > 1. Add setZoomLevel()/getZoomLevel() methods to SapphireDiagramEditorPagePart. > Probably use an integer. Similar to setGridVisible()/isGridVisible(). Just > instead of using the deprecated listener API, it should use the new > (o.e.s.Listener) system. Let's define ZoomLevelEvent for that. The rest should > be straightforward use of attach/detach/broadcast API. > > 2. Move zoom action to the main UI plugin and have them use the new zoom level > API. > > 3. Make SapphireDiagramEditor listener on ZoomLevelEvent and coordinate with > GEF zoom manager. > > Let me know if anything above is unclear. These are all done now. here are a couple of notes: - I set the zoom level initial value to 100, to represent 100% zoom level - the zoom-in/out goes at increments of 25 (with the thought towards that is how GEF zoom manager zooms at .25 increments) - zoom actual in both toolbar and context menu is correctly disabled when zoom level is 100 - The zoom level below 50 is ineffectual for GEF renderer. We could put hard limit on the zoom level, like nothing below 0 and nothing about 200 and also add isEnabled() checks for those bounds, but I didn't want to introduce this behavior until it was discussed.
Accepted Patch v5 with following changes: 1. Implemented constraining of zoom levels between 50% and 200% to line up with GEF's constraints per Greg's suggestion. Definitely a necessary behavior. In the future when we support multiple presentations, we will need to figure out how to handle different limits. Not a concern today. 2. Improved how zoom action enablement is handled by calling setEnabled() method instead of overriding isEnabled(). 3. Fixed a couple of issues in save as image action handler... (A) The getRawLocation() method returns null for projects at the default location in the workspace. I was getting an NPE for a file in project root. It should have been getLocation(). (B) Removed an assumption that the editor input file is a .xml file. Remaining items: 1. I noticed that when zoom level is anything other than 100%, moving the connection label does not work correctly. The connection label jumps somewhere other than where it was dropped. Moving nodes, creating nodes, moving bendpoints and creating bendpoints all works correctly. Who wants to take a look at this one? 2. An entry is needed in the enhancement doc. 3. The zoom level should be persisted in the editor state cookie. See how this is done in MasterDetailsEditorPage.setDetailsMaximized(). Note that I would not expect this to be stored in the diagram layout persistence.
I'll submit remaining items #2 and #3 as a new patch.
Created attachment 215927 [details] Patch v6
Created attachment 215928 [details] Patch Phase 2 v1 Images
Created attachment 215934 [details] Patch Phase 2 v1
I've attached a patch v6a for remaining items #2 and #3. A couple of notes: * I had to add two new packages to the exported packages list for org.eclipse.sapphire.ui, when it did that the manifest.mf editor changed the order of things just a bit * Had to add a null-check to avoid a NPE that was happening in SapphirePart#adapt() * There is a collision of state files with the current getDefaultStateStorageFile method. Currently it looks up in a map (pagesById) to get the key of the current page state file that is being located. However, at the time when the getDefaultStateStorageFile is called for both the diagram page and a master details page (init() and constructor() respectively), the pagesById map is empty. So the digest path for these files always ends with '#' and not the actual page name. So if you look at map.xml editor, you wil see that the state digest path for masterdetailspage ends with '#' and not the expected #Overview like the code is built for. So in my use of this method for getting the state file for the SapphireDiagramEditorPagePart page I was getting a collision and the same state file is used for MasterDetails and Diagram pages. My fix was to use a different approach than the pagesById map and that is to use the ID of the part that is passed into this method. So I have modified the implemention of getDefaultStateStorageFile() to take an ISapphirePart and then use the ID of that part definition and if that is null then use the type of the part definition type simple name. So this is a bit unstable because ID for a part is not required which means it will fall back to part definition type sometimes. So the drawback will be if a sapphire adopter changes either their ID or their type definition for parts between releases, the downstream users would lose state after an update. An alternative would be to pass in a suffix to the getDefaultStateStorageFile so it could be appended to the default key name for calculating the digest to help with avoiding collisions.
(In reply to comment #23) > 1. I noticed that when zoom level is anything other than 100%, moving the > connection label does not work correctly. The connection label jumps somewhere > other than where it was dropped. Moving nodes, creating nodes, moving > bendpoints and creating bendpoints all works correctly. Who wants to take a > look at this one? > Bug https://bugs.eclipse.org/bugs/show_bug.cgi?id=380163 tracks this problem.
*** Bug 380163 has been marked as a duplicate of this bug. ***
I accepted Patch Phase 2 v1 in part... > * I had to add two new packages to the exported packages list for > org.eclipse.sapphire.ui, The internal package does not need to be exported. > * There is a collision of state files with the current > getDefaultStateStorageFile method. Yes, it looks like the current implementation has regressed at some point. Using the Id doesn't work all that well, but using the PageName property should do the trick. In the future, when sdef has representation of how an editor is assembled from pages, this will need to be more formally addressed.
Checked in fix for moving labels in a zoomed state.
Marking this as fixed. > Checked in fix for moving labels in a zoomed state. I verified this aspect. Ling or Shenxue, could one of you verify the rest?
(In reply to comment #33) > Ling or Shenxue, could one of you verify the rest? Let me hook the print menu first.
Print menu has been enabled for the diagram page.
Verified zoom, print and save as image in map.xml.
