[JBIDE-19717] Visual (diagram) editor for JSR-352 batch job files

[tommilata]

Features

• Visualization of structure of batch job, i.e.:
• step, flow, split, decision
• connection using next attribute and next elements
• end, stop and fail outcomes
• support for browsing nested flows
• Other properties editable via context actions and linked property views
• Setting a flow element as a start (i.e. reordering)
• Content proposal for batch artifacts

See video https://youtu.be/wmWFQKvTWSc

Code Changes

• Most of the existing Sapphire model was reused.
  • NextPossibleValueService replaced with Sapphire ReferenceService which provides object-oriented support instead just String values.
  • Separate model fields for next attributes and terminating outcomes (end, fail, stop). They need to be handled separately in diagram. The result is that next attributes are accesible via two fields but that is no problem for Sapphire.
• Content proposal classes only delegate existing support from IBatchProject directly to Sapphire model.
• Navigation through nested flows using custom Sapphire actions.
• Since Sapphire is not expressive enough for connections using next attribute, a custom connection service was implemented.

Sapphire version

• Due to a bug in Sapphire, the diagram will not work with Sapphire builds older than https://hudson.eclipse.org/sapphire/job/8.2.x/52/. See
  https://www.eclipse.org/forums/index.php/t/1065375/ and https://bugs.eclipse.org/bugs/show_bug.cgi?id=463917.

Notes

• History of builds during all commits from Travis CI https://travis-ci.org/tomas-milata/jbosstools-javaee/builds

[maxandersen]

This looks great! Will take some time to review but want to say Good Job.

[alexeykazakov]

Yes, good job! I watched the video and it looks great. We are starting to review the code and will comment any problem we found here.

[alexeykazakov]

1.) Layout is not saved/restored when you edit and close/re-open the editor.
If I move any elements on the diagram and re-open the editor the position of elements are not restored.
Does sapphire provides such feature to save the layout of the diagram?

[alexeykazakov]

2.) (UPD: FIXED) Add step1 and step 2 to the diagram. Add a "next" transition from step1 to step2. Go to the source tab and rename step2 to step3, change next="step3" too. Switch to the diagram. The "next" transition is gone (a sapphire bug?). Re-open the editor. Here we go. The transition is visible again.

[tommilata]

1. Unfortunately, the standard Sapphire layout persistence is not applicable to our custom connection model.

It is definitely possible to write a custom diagram layout persistence service but it will require writing some hundreds lines of code. Due to my lack of time last days and low priority I skipped this feature as a possibility for future enhancements. (And also due to the fact that the default layout after reopen seems quite nice, you can even choose vertical/horizontal).

[tommilata]

2. Confirmed and investigating... Thanks.

[alexeykazakov]

3. There is no enough space for icons in my ubuntu (not critical though):

[scabanovich]

4. I play with this content inside root element:

<decision id="ssdd" ref="myDecider" >
    <next to="s2" on="oo"/>
</decision>
<step id="a0" next="ssdd">
</step>
<step id="s2" next="dd"></step>
<decision id="dd" ref="myDecider"></decision>    

what I see on diagram depends very much on what I do to get to this code.
Sometimes I have a0->ssdd and s0->dd, but no ssdd->s2.
Sometimes I have ssdd->s2 only.
Sometimes I have all, but ssdd->s2 has 3 copies.

[tommilata]

3. I posted a question on Sapphire forum. https://www.eclipse.org/forums/index.php/t/1066202/

Maybe you could add more details about your environment to the forum in case they verify it is a Sapphire bug.

[dazarov]

5. If you have Palette View opened - you will get double palette, one in the editor, another in the Palette View. See -
   

[alexeykazakov]

I don't think 5) is a big problem. Especially if the palette in the editor can be folded. User can decide which palette he/she likes better to use.

[tommilata]

2. I have pushed a fix.

There was a bug in the lifecycle of connections (nodes added after initialization of connection service were not notified about changes of attributes so I added missing listeners.)

Thank you for finding the bug.

[tommilata]

4. Could you, please, verify whether the problem is gone or persists after the new fixes for 2) ? If it is not gone, could you please describe how exactly did you get the unexpected result? Thanks a lot.

[scabanovich]

On (4)

I create xml file with content.

<job id="myJob" xmlns="http://xmlns.jcp.org/xml/ns/javaee"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/jobXML_1_0.xsd" version="1.0">
<step id="a0" next="ssdd">
</step>
<decision id="ssdd" ref="myDecider">
<next to="s2" on="oo"/>
</decision>
<step id="s2" next="dd"></step>
<decision id="dd" ref="myDecider"></decision>
</job>

Then I close and restart Eclipse and open the file. I get the last described variant with 3 copies of middle transition.

If you cannot get that, probably it is fixed in Sapphire or Gef recently which means we have to rise versions to use the diagram.

[scabanovich]

6. This is on model improvement rather than on diagram.
   Now there are 12 declarations of property
   ValueProperty PROP_REF = new ValueProperty( TYPE, "Ref" );
   in model interfaces and 11 subclasses of AbstractBatchContentProposalService.
   We can avoid that with the only property declaration in interface

public interface RefAttr {
ElementType TYPE = new ElementType( RefAttr.class );

@Label( standard = "ref" )
@XmlBinding( path = "@ref" )
@Required
@Service( impl = RefProposalService.class )
ValueProperty PROP_REF = new ValueProperty( TYPE, "Ref" );

Value<String> getRef();
void setRef( String ref);

}

and the only service RefProposalService.

New interface will be used in any model interface that needs 'ref' attribute, e.g.

public interface Decision extends FlowElement, RefAttr ...

Sapphire will find and implement this attribute.

Now 11 subclassed services provide kind of mapping between model interfaces and BatchArtifactType. But then that mapping remains incapsulated in this service and non-reusable, while actually we might need it in other services. An utility class may keep Map<Class<? extends RefAttr>, BatchArtifactType> and provide method

BatchArtifactType getArtifactType(Class<? extends RefAttr> cls)

Then RefProposalService can get from context current element type and call the utility method.
Independent computation of image is not needed, there should be a way to request it from the current element.

Profits:
11 propety declarations less,
11 classes less,
300 code lines less
Mapping (Model object) -> (Artifact Type) is reusable.

[tommilata]

4. OK, this case works correctly after the last fixes and with correct sapphire version.

[tommilata]

6. Thats a good point. Just one detail, the mapping must return a list of BatchArtifactTypes, not just one. because step listener may refer to many kinds of listeners (STEP_LISTENER, ITEM_READ_LISTENER, RETRY_READ_LISTENER, etc. )... we do not have one enum that would group them all, right?

I will push an update of this tomorrow.

[scabanovich]

It may be a list, or the case of StepListener be treated as a special one.

[tommilata]

6. I pushed the proposed update so please review.

[maxandersen]

About 3) I think "Go Back to Parent" should be replaced by an icon/button instead. like https://www.dropbox.com/s/j2h0fkqpqh55yed/Screenshot%202015-05-03%2012.51.40.png?dl=0 which is used in project explorer when used "go into" folder.

[tommilata]

@maxandersen And how about adding this icon and keeping a label (maybe shorter one) next to the icon? There can be both at the same time. I'm just afraid that such an important navigation button could get lost among others (not so important like "print" or "zoom").

E.g. this:

[maxandersen]

that approach is definitely better than the current one. Maybe the icon should be moved near/next to the title of the job so it becomes apparent it is not related to zoom/print.

[tommilata]

I'm afraid Sapphire does not allow this. From all buttons in the title bar,I can put this one at the first place on the left, as it is, but I cannot move the whole section with buttons, it will be always aligned right.

[tommilata]

So I pushed the version with icon and 'Parent' label.

[scabanovich]

6. Great.
7. property 'restart' of element 'stop' references by id elements 'step', 'flow', 'split' (but not 'decision') on the job-level. So, like 'next' attribute, this one may be made @ElementReference, referencing new property RestartableFlowElements differing from FlowElements by excluding Decision objects. Path is taken from root, not relative to the current element.
   On diagram, when Job level is shown, 'restart' may be displayed as a transition.
   It may be good to have in Palette transition section a tool for creating restart transition, only does Sapphire allow transitions from child elements inside parent node, as is the case with 'stop'?

[tommilata]

7. Well, since the transition cannot be displayed as an arrow in the nested flows (because the target element is in the top-level), I guess we should add a label with id of the restart target.

Wouln't it be more consistent and less confusing then if it was done the same way everywhere (even on top-level)?

[alexeykazakov]

Sorry, I didn't realize you need to merge this PR this week.
Two more things:

1. Please squash all these 74 commits. These commits are too atomic. It's OK to have a few commits for big changes like this one, but 74 commits is too many. So please squash them into one (or a few if you think it will make sense).
2. All commits message should start with JBIDE-19717. If you squash all commits into the only one then it could have a message "JBIDE-19717 Visual (diagram) editor for JSR-352 batch job files" or something like that.
   As soon as you done with these I will be ready to merge your PR to master.

[alexeykazakov]

And one more:
3. All new files should have EPL in their header:
For example for JAVA files:

/******************************************************************************* 
 * This program is made available under the terms of the 
 * Eclipse Public License v1.0 which accompanies this distribution, 
 * and is available at http://www.eclipse.org/legal/epl-v10.html 
 * 
 * Contributors: 
 * Tomáš Milata - initial API and implementation 
 ******************************************************************************/

[tommilata]

@alexeykazakov License added and commits squashed. Thanks.

[alexeykazakov]

Batch Editor is broken now:
java.lang.IllegalArgumentException
at org.eclipse.sapphire.ui.def.DefinitionLoader.page(DefinitionLoader.java:203)
at org.jboss.tools.batch.ui.editor.internal.model.JobXMLEditor.createDiagramPages(JobXMLEditor.java:63)
at org.eclipse.sapphire.ui.SapphireEditor.addPages(SapphireEditor.java:487)
at org.eclipse.ui.forms.editor.FormEditor.createPages(FormEditor.java:138)

[tommilata]

@alexeykazakov Oh, sorry about that. It is fixed now. I have malformed the XMLs when adding license header... From now on I'll remember to run tests even after adding comments.

Editor is running and tests passing. Thanks.