Bugzilla – Full Text Bug Listing
| Summary: | Allow customization of diagram palette compartments | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Shenxue Zhou <shenxue.zhou> |
| Component: | Sapphire | Assignee: | Shenxue Zhou <shenxue.zhou> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | konstantin |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Shenxue Zhou
Let's group these settings as there are likely to be more of these in the future... <palette> <nodes-group-label> <connections-group-label> </palette> Fixed as suggested In reviewing this change, I came across another related api...
<node>
<tool-palette-location>connection</tool-palette-location>
</node>
Considering both of these together, I believe this API needs another round of design...
It seems that the way this API is designed allows the developer to "subvert" the intentions for these bins. It would be better if we just opened up the API to make it easier to define palette bins.
Let's try this for size...
At diagram page level, we would have this:
<diagram-page>
<palette>
<bin>
<id>Something</id>
<label>Something Else</label>
</bin>
</palette>
</diagram-page>
Then at node/connection level, let's have a "palette-bin" attribute that reference the id of the palette bin.
By default, if no palette bins are defined, we would have two bins (Sapphire.Diagram.Palette.Nodes and Sapphire.Diagram.Palette.Connections). These ids are then the default values for the PaletteBin property at node/connection level.
Feel free to use the term bin or whatever other term you think is appropriate (group, bucket, whatever). Maybe GEF has an established term for these?
-----------
Unrelated comment... In the enhancement doc, you reference "user". That can be a bit confusing. Are you referencing the user of the framework or the user that will be using the UI written with the framework. It would be a bit less ambiguous to use the term "developer".
Meant to re-open. *** Bug 356537 has been marked as a duplicate of this bug. *** (In reply to comment #3) > In reviewing this change, I came across another related api... > > <node> > <tool-palette-location>connection</tool-palette-location> > </node> > > Considering both of these together, I believe this API needs another round of > design... > > It seems that the way this API is designed allows the developer to "subvert" > the intentions for these bins. It would be better if we just opened up the API > to make it easier to define palette bins. > > Let's try this for size... > > At diagram page level, we would have this: > > <diagram-page> > <palette> > <bin> > <id>Something</id> > <label>Something Else</label> > </bin> > </palette> > </diagram-page> > > Then at node/connection level, let's have a "palette-bin" attribute that > reference the id of the palette bin. > > By default, if no palette bins are defined, we would have two bins > (Sapphire.Diagram.Palette.Nodes and Sapphire.Diagram.Palette.Connections). > These ids are then the default values for the PaletteBin property at > node/connection level. > > Feel free to use the term bin or whatever other term you think is appropriate > (group, bucket, whatever). Maybe GEF has an established term for these? > I think GEF uses "compartment" for that. For every tool in the palette, I need to associate it with either a node edit part or connection edit part. So logically it makes sense to group all the connection edit parts into a connection "bin" and all the node edit parts into an object "bin". The problem of generalizing palette definition is how to create the linkage between a "bin" and connection edit parts/node edit parts. I can't support a generalized bin without association with nodes/connections. If we can make the "bin" id uniquely identify node/connection groups: <diagram-page> <palette> <compartment> <id>connections</id> <label>my connections</label> </compartment> </palette> <palette> <compartment> <id>nodes</id> <label>my objects</label> </compartment> </palette> </diagram-page> And in node/connection definition, we allow them to go under a different compartment by referring to the compartment id: <node> <tool-palette-compartment>connections</tool-palette-location> </node> This is not much different from what's implemented already. (In reply to comment #6) > (In reply to comment #3) > > In reviewing this change, I came across another related api... > > > > <node> > > <tool-palette-location>connection</tool-palette-location> > > </node> > > > > Considering both of these together, I believe this API needs another round of > > design... > > > > It seems that the way this API is designed allows the developer to "subvert" > > the intentions for these bins. It would be better if we just opened up the API > > to make it easier to define palette bins. > > > > Let's try this for size... > > > > At diagram page level, we would have this: > > > > <diagram-page> > > <palette> > > <bin> > > <id>Something</id> > > <label>Something Else</label> > > </bin> > > </palette> > > </diagram-page> > > > > Then at node/connection level, let's have a "palette-bin" attribute that > > reference the id of the palette bin. > > > > By default, if no palette bins are defined, we would have two bins > > (Sapphire.Diagram.Palette.Nodes and Sapphire.Diagram.Palette.Connections). > > These ids are then the default values for the PaletteBin property at > > node/connection level. > > > > Feel free to use the term bin or whatever other term you think is appropriate > > (group, bucket, whatever). Maybe GEF has an established term for these? > > > > I think GEF uses "compartment" for that. > > For every tool in the palette, I need to associate it with either a node edit > part or connection edit part. So logically it makes sense to group all the > connection edit parts into a connection "bin" and all the node edit parts into > an object "bin". The problem of generalizing palette definition is how to > create the linkage between a "bin" and connection edit parts/node edit parts. I > can't support a generalized bin without association with nodes/connections. > > If we can make the "bin" id uniquely identify node/connection groups: > > <diagram-page> > <palette> > <compartment> > <id>connections</id> > <label>my connections</label> > </compartment> > </palette> > <palette> > <compartment> > <id>nodes</id> > <label>my objects</label> > </compartment> > </palette> > </diagram-page> > > And in node/connection definition, we allow them to go under a different > compartment by referring to the compartment id: > > <node> > <tool-palette-compartment>connections</tool-palette-location> > </node> > > > This is not much different from what's implemented already. I meant the following for the palette definition: <diagram-page> <palette> <compartment> <id>connections</id> <label>my connections</label> </compartment> <compartment> <id>nodes</id> <label>my objects</label> </compartment> </palette> </diagram-page> It sounds like we are on the same page here. You define a compartment with the id. You then reference the compartment in node/connection definition. I like the term compartment. Let's use that. Did another round of cleanup... Sigh... unfortunately you aren't done yet. 1. The ToolPaleteCompartmentId properties are enums. That is incorrect. They should be String value properties with possible values hookup. 2. The two sections in what's new doc should be a single section as this is really one feature. 3. In sdef editor, the "Diagram Editor Page" section looks bad. We should at least make some effort to present parts in a cleaner manner. Maybe create a separate "Palette" section? 4. IDiagramPaletteCompartmentDef.CompartmentId/CompartmentLabel repeat the term compartment in properties, even though it is obvious from context. These should be simply Id and Label. 5. The properties in IDiagramPaletteCompartmentDef should both be marked as @Required. 6. The property IDiagramPaletteCompartmentDef.CompartmentId is missing @Label annotation which cause default "compartment id" label to be used. The label should be specified and it should be specified as "id". Meant to re-open. Fixed. Still not finished. 1. PaletteCompartmentIdService should not be attached to IDiagramPaletteCompartmentDef.Id. The point of defining palette compartments is to specify new ones. Developers aren't going to be picking from the list of existing compartments here. 2. The implementation of PaletteCompartmentIdService as applied to Connection/Node ToolPaletteCompartmentId properties is clearly wrong. It needs to draw it's values from the set of defined palette compartments and only if no compartments are defined, should it generate the default list. 3. The ids of default compartments should not be simply "nodes" and "connections". We should use namespace qualified form to avoid confusion with possible user-defined compartments. Let's use "Sapphire.Diagram.Palette.Nodes and Sapphire.Diagram.Palette.Connections" as stated in my Comment #3. I hate getting into this string of resolve/re-open. Let's chat offline about this. Addressed issues raised in Comment 13... The API looks good now, but I hit the following exception during a functional test: The immediate issue is that the message id is misspelled in the .properties file, but this shouldn't be an exception in the first place. It would be better to find a reasonable fallback strategy that doesn't prevent the editor from opening. For instance... use the first defined compartment. Method 'setInput(IEditorInput)': Can not open the modifier. Details NLS missing message: invalidCompartmentIdMsg in: org.eclipse.sapphire.ui.swt.graphiti.providers.SapphireDiagramToolBehaviorProvider Details: java.lang.IllegalArgumentException: NLS missing message: invalidCompartmentIdMsg in: org.eclipse.sapphire.ui.swt.graphiti.providers.SapphireDiagramToolBehaviorProvider at org.eclipse.sapphire.ui.swt.graphiti.providers.SapphireDiagramToolBehaviorProvider.getPalette(SapphireDiagramToolBehaviorProvider.java:238) at org.eclipse.graphiti.ui.internal.editor.GFPaletteRoot.updatePaletteEntries(GFPaletteRoot.java:104) at org.eclipse.graphiti.ui.internal.editor.GFPaletteRoot.<init>(GFPaletteRoot.java:79) at org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.createPaletteRoot(DiagramEditorInternal.java:459) at org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.getPaletteRoot(DiagramEditorInternal.java:923) at org.eclipse.gef.ui.parts.GraphicalEditorWithFlyoutPalette.setEditDomain(GraphicalEditorWithFlyoutPalette.java:145) at org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.setConfigurationProvider(DiagramEditorInternal.java:1593) at org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.setInput(DiagramEditorInternal.java:618) at org.eclipse.sapphire.ui.swt.graphiti.editor.SapphireDiagramEditor.setInput(SapphireDiagramEditor.java:367) at org.eclipse.gef.ui.parts.GraphicalEditor.init(GraphicalEditor.java:346) at org.eclipse.graphiti.ui.internal.editor.DiagramEditorInternal.init(DiagramEditorInternal.java:1093) at org.eclipse.ui.part.MultiPageEditorPart.addPage(MultiPageEditorPart.java:237) at org.eclipse.ui.forms.editor.FormEditor.addPage(FormEditor.java:325) at org.eclipse.sapphire.samples.map.ui.MapEditor.createDiagramPages(MapEditor.java:83) at org.eclipse.sapphire.ui.SapphireEditor.addPages(SapphireEditor.java:373) at org.eclipse.ui.forms.editor.FormEditor.createPages(FormEditor.java:138) at org.eclipse.ui.part.MultiPageEditorPart.createPartControl(MultiPageEditorPart.java:348) at org.eclipse.ui.internal.EditorReference.createPartHelper(EditorReference.java:670) at org.eclipse.ui.internal.EditorReference.createPart(EditorReference.java:465) at org.eclipse.ui.internal.WorkbenchPartReference.getPart(WorkbenchPartReference.java:595) at org.eclipse.ui.internal.EditorAreaHelper.setVisibleEditor(EditorAreaHelper.java:271) at org.eclipse.ui.internal.EditorManager.setVisibleEditor(EditorManager.java:1459) at org.eclipse.ui.internal.EditorManager$5.runWithException(EditorManager.java:972) at org.eclipse.ui.internal.StartupThreading$StartupRunnable.run(StartupThreading.java:31) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4140) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3757) at org.eclipse.ui.application.WorkbenchAdvisor.openWindows(WorkbenchAdvisor.java:803) at org.eclipse.ui.internal.Workbench$33.runWithException(Workbench.java:1595) at org.eclipse.ui.internal.StartupThreading$StartupRunnable.run(StartupThreading.java:31) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4140) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3757) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2604) 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) Other issues: 1. PaletteCompartment property in node/connection doesn't define dependency on PaletteCompartments/Id, so change palette compartments doesn't trigger re-validation of the references. 2. Palette compartment label isn't passed through the capitalization algorithm like all labels should be. For instance, specifying "foo bar" in sdef for compartment label shows "foo bar" in the editor instead of expected "Foo Bar". When you fix this make sure to updated the default compartment labels to be in all-lowercase. The enhancement doc section needs to be updated based on the last set of changes. The current text talks about changing labels of existing compartments which isn't how this feature works now. I have made some cleanup changes to API and sdef editor related to this feature. Make sure to sync to latest before taking on the next round of changes. Issues in comment 15, 16 and 17 have been addressed Verified. Closing. |