Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 344053 - [diagram] Replace Graphiti actions with Sapphire ones
Summary: [diagram] Replace Graphiti actions with Sapphire ones
Status: CLOSED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Sapphire (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: ---   Edit
Assignee: Shenxue Zhou CLA
QA Contact:
URL:
Whiteboard:
Keywords: usability
: 344303 344320 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-04-27 18:15 EDT by Konstantin Komissarchik CLA
Modified: 2021-11-19 09:22 EST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Komissarchik CLA 2011-04-27 18:15:06 EDT
All Graphiti actions should be removed from editor toolbars and context menus. They should be replaced with actions from Sapphire actions system (sdef file and extension system).
Comment 1 Konstantin Komissarchik CLA 2011-04-29 13:07:46 EDT
*** Bug 344303 has been marked as a duplicate of this bug. ***
Comment 2 Konstantin Komissarchik CLA 2011-04-29 13:27:57 EDT
*** Bug 344320 has been marked as a duplicate of this bug. ***
Comment 3 Shenxue Zhou CLA 2011-05-17 13:03:33 EDT
1. Now we take full control of the diagram and node context menus. They have been replaced with sapphire actions.
2. Diagram context menu is made up of "add" actions for each node type. We'll need to add "print", "zoom", "save as image" actions later.
2. The floating palette of a node is composed of "delete", "show in source" and "create connection" actions. "Create connection" tool palette is an effective way of creating a connection without switching to connection creation mode.
3. Node context menu has node default action, "show in source" action and other actions specified using sapphire action declaration mechanism (extension and sdef)

When we get the Graphiti bug fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=345347 into our build system, we can mark this bug as "fixed".
Comment 4 Konstantin Komissarchik CLA 2011-05-17 13:06:14 EDT
Good timing. I just made the change to the build to pickup Graphiti 0.8 RC1, which should have the fix in question. Please sync up, build, verify and resolve.
Comment 5 Konstantin Komissarchik CLA 2011-05-17 15:43:25 EDT
Tried this with the map sample. The editor context menu had an empty child menu item under Add. The following exception in the log. Also, a cascading menu under add when there is only one handler (one node type) is not correct. See how this works in the form editors content outline node add action.

java.lang.NullPointerException
	at org.eclipse.sapphire.ui.diagram.actions.DiagramNodeAddActionHandler.getImageId(DiagramNodeAddActionHandler.java:61)
	at org.eclipse.sapphire.ui.swt.graphiti.actions.AddNodeAction.getImageDescriptor(AddNodeAction.java:64)
	at org.eclipse.jface.action.ActionContributionItem.updateImages(ActionContributionItem.java:1103)
	at org.eclipse.jface.action.ActionContributionItem.update(ActionContributionItem.java:942)
	at org.eclipse.jface.action.ActionContributionItem.fill(ActionContributionItem.java:291)
	at org.eclipse.jface.action.MenuManager.doItemFill(MenuManager.java:737)
	at org.eclipse.jface.action.MenuManager.update(MenuManager.java:818)
	at org.eclipse.jface.action.MenuManager.handleAboutToShow(MenuManager.java:470)
	at org.eclipse.jface.action.MenuManager.access$1(MenuManager.java:465)
	at org.eclipse.jface.action.MenuManager$2.menuShown(MenuManager.java:491)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:247)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1053)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1077)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1058)
	at org.eclipse.swt.widgets.Control.WM_INITMENUPOPUP(Control.java:4864)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4540)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:341)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1610)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2059)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:4970)
	at org.eclipse.swt.internal.win32.OS.TrackPopupMenu(Native Method)
	at org.eclipse.swt.widgets.Menu._setVisible(Menu.java:256)
	at org.eclipse.swt.widgets.Display.runPopups(Display.java:4204)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3746)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2696)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2660)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2494)
	at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:674)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:667)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:344)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1410)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1386)
Comment 6 Konstantin Komissarchik CLA 2011-05-17 15:51:19 EDT
Other notes:

1. Delete action on the floating toolbar must use the image and text specified for Sapphire.Delete action, not the trashcan icon.

2. The create connection action in the floating toolbar is missing an image. I just get a small red square.

3. There is a separate button for each connection type. This should be implemented as multiple action handlers of one action. That is, a single button with a cascading menu.

4. Clicking on creation connection buttons doesn't seem to do anything. Nothing in the log either. 

5. I'd like to see the connect action surfaced in the node context menu. We can keep it in the floating toolbar as well. At least until we get more experience on which action are appropriate in which context.

Note that I was using Graphiti 0.8 RC1 when trying this out, in case some of these are result of some changes in Graphiti.
Comment 7 Konstantin Komissarchik CLA 2011-05-17 15:52:17 EDT
> We'll need to add "print", "zoom", "save as image" actions later.

Please open enhancement requests for each of these and any other that come to mind later.
Comment 8 Konstantin Komissarchik CLA 2011-05-17 15:54:46 EDT
What about the context menu on the connection and the connection bendpoints. We have other bugs opened for specific actions on bendpoints, but I would at least like to see "Show In Source" on the connection.
Comment 9 Shenxue Zhou CLA 2011-05-17 16:24:18 EDT
1. The NPE is a bug. I have a fix for it. If there is only one node type, we don't need a cascade menu. I'll fix that.

2. The "Delete" action in the node floating palette comes with Graphiti. It does allow us to override its semantic meaning. The keyboard shortcut is the standard "Del" key. It has been working since the beginning of the Sapphire Diagram Framework. So I didn't try to retrofit it into Sapphire Action mechanism. I did remove other Graphiti actions in the node floating palette and add "show in source" action there. 

3. The missing image of "creating connection" is intentional. If you see connection image in the diagram editor tool palette, that's the image I use for the node floating palette. If there is no image associated with the tool palette, then Eclipse's "missing" icon is used. If I don't use the missing icon, then an empty space is shown instead. The way this feature works is you click on it and then drag it to other node to create a connection. The nice thing about it is it won't force you to switch to connection creation mode and then have to switch back to "marquee" to select any node.

4. The floating palette is Graphiti specific feature. I've looked at its API and it doesn't seem possible to render it in Sapphire Action Group style (single button with a cascading menu)

5. I have to investigate diagram context menu for creating connections. The node floating palette for creating connection seems to be more direct than diagram context menu. You only need to click on the floating palette and then start dragging it to the other node since you already have a starting node.

6. I'll add "Show In Source" in the connection context menu. It's an oversight.
Comment 10 Konstantin Komissarchik CLA 2011-05-17 17:15:47 EDT
> 2. The "Delete" action in the node floating palette comes with Graphiti. It
> does allow us to override its semantic meaning. The keyboard shortcut is the
> standard "Del" key. It has been working since the beginning of the Sapphire
> Diagram Framework. So I didn't try to retrofit it into Sapphire Action
> mechanism. I did remove other Graphiti actions in the node floating palette 
> and add "show in source" action there. 

lease get rid of the Graphiti delete action and replace it with Sapphire one. Key binding needs to be surfaced as specified for the given Sapphire action. That is, we cannot rely on any hardcodded Graphiti feature for this. See SapphireKeyboardActionPresentation.

> 3. The missing image of "creating connection" is intentional. If you see
> connection image in the diagram editor tool palette, that's the image I use 
> for the node floating palette. If there is no image associated with the tool
> palette, then Eclipse's "missing" icon is used. If I don't use the missing
> icon, then an empty space is shown instead. 

Please find a default image to use for the connect action. The missing image icon is not an acceptable default. Search for candidate images in Eclipse plugins. Any art you find in Eclipse plugins is fair game, but don't go outside of that as that will invoke IP process. If you find an image that is close, but not perfect, I may be able to modify it to fit our use.

> The way this feature works is you
> click on it and then drag it to other node to create a connection. The nice
> thing about it is it won't force you to switch to connection creation mode and
> then have to switch back to "marquee" to select any node.

> 5. I have to investigate diagram context menu for creating connections. The
> node floating palette for creating connection seems to be more direct than
> diagram context menu. You only need to click on the floating palette and then
> start dragging it to the other node since you already have a starting node.

This is interesting, but unfortunately not intuitive. People don't expect to drag a button. They expect to click on it. The way I expect the connect action to work is you press a button or a menu item. That immediately establishes a partial connection. The user should see a line from the node to their cursor at this point. Then they just have to terminate it. At that point, the connection creation state should cease.

> 4. The floating palette is Graphiti specific feature. I've looked at its API
> and it doesn't seem possible to render it in Sapphire Action Group style
> (single button with a cascading menu)

We need to figure out a way to do this. Placing every handler as a separate button doesn't scale. In Graphiti toolbar API, is there a way for you to get coordinates of the button? If so, you should have everything you need to implement a button menu. See SapphireToolBarActionPresentation.
Comment 11 Shenxue Zhou CLA 2011-05-17 19:09:20 EDT
I checked in a connection image for the node floating palette. It's designed by Troy and used by OEPE. It's at org.eclipse.sapphire.ui.swt.graphiti/icons dir. The only thing is it's greenish color. The default node color in sapphire diagram is (51, 51, 153). We probably should use a color close to that. 

Feel free to modify it if you have a good image tool. Otherwise I can ask Troy to design one when he gets back next week.
Comment 12 Konstantin Komissarchik CLA 2011-05-17 19:43:16 EDT
Verified the fix for NPE, no cascading add menu when there is only one handler and the new icon for the connect action.

Please go ahead and ping Troy about designing an icon for connect action. The issue isn't just with color. An arrow works to represent a noun of "connection", but doesn't work as well to represent a verb of "connect". We need a different base concept.

Two other issues:

1. For some reason the node add menu item lacks an image, yet an image is defined for the Sapphire.Add action...

2. I have twice got into a state where I have a node and its floating toolbar show Graphiti delete action and Graphiti remove action (eraser). No other buttons. Around the same time (may be connected), I also noticed that in certain cases moving a node resulted in duplication of said node rather than movement. Both behaviors are sporadic. I don't have a consistent repro and there was nothing in the log. I was using the map sample.
Comment 13 Shenxue Zhou CLA 2011-05-24 12:21:35 EDT
Sapphire actions and handlers are surfaced in the diagram context menu and diagram node/connection context menu now.

I'll open another enhancement bug to keep track of the work needed to implement sapphire action key bindings in diagram editor.
Comment 14 Shenxue Zhou CLA 2011-06-02 16:12:02 EDT
The print, zoom actions are tracked by https://bugs.eclipse.org/bugs/show_bug.cgi?id=346172
Comment 15 Ram Venkataswamy CLA 2011-06-03 13:45:21 EDT
verified with build 372
Comment 16 Konstantin Komissarchik CLA 2011-06-03 15:41:21 EDT
This is not complete. I still see Graphiti delete action in the floating toolbar as well as the "drag a button" method of creating connections.
Comment 17 Shenxue Zhou CLA 2011-06-03 16:24:30 EDT
(In reply to comment #16)
> This is not complete. I still see Graphiti delete action in the floating
> toolbar as well as the "drag a button" method of creating connections.

I just commented out the floating palette from sapphire diagram. I left it in for the purpose of receiving feedback.
Comment 18 Konstantin Komissarchik CLA 2011-06-03 16:50:06 EDT
As we previously discussed, I am fine with disabling the floating toolbar as a workaround for resolving these issues for the 0.3 release, but please open another bug to bring back the floating toolbar and a bug to implement connection creation action.
Comment 19 Ram Venkataswamy CLA 2011-06-03 18:24:11 EDT
verified with build 376
Comment 20 Konstantin Komissarchik CLA 2011-06-09 16:36:13 EDT
Closing.