[WIP] Merge KEITH features #49

soerendomroes · 2020-07-09T06:49:44Z

Can only be merged after an ELK release and should use ELK 0.7.0, Xtext 2.20 and at least eclipse 2019-12.

…s/constraints

Empty stub for setStaticConstraint was added to the LanguageServerExtension. refreshLayout was added to the LanguageServerExtension and implemented. It is called in order that nodes snap back if no constraint was set. Additionally, methods were added to shorten the retrieval and setting of constraints.

The last commit did not work without the StaticConstraint class that I forgot to commit

This extension point allows any projects wanting to extend any KLighD behavior for different run configurations to only need to register this extension once via Eclipse extension and via Java ServiceLoader in META-INF/services and programatically register everything needed in the implementation of this extension with the KLighD API for registering extensions programmatically.

Instead of throwing exceptions, a message model displaying that no model is in the editor is shown.

generic one.

Merge remote-tracking branch 'origin/keith' into mainThreadQueue Conflicts: plugins/de.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/KGraphLanguageServerExtension.xtend

klighd.lsp: Added main thread queue for SWT and AWT calls.

Moved the initialization and loading of all extensions into the static part after creating the instance, so that the startupHook extension may use the instance and modify it directly.

sailingKieler

I found a few things, none of them are major things, of course, but please have a look and fix esp. the out-dated javadocs.

sailingKieler · 2020-09-09T15:59:30Z

...ins/de.cau.cs.kieler.kgraph.text/src/de/cau/cs/kieler/kgraph/text/KGraphStandaloneSetup.java

+        if (injector == null) {
+            injector = new KGraphStandaloneSetup().createInjectorAndDoEMFRegistration();
+        }
+        return injector;


AFAIK keeping the injector in a field is not required, as it's kept in KGraphStandaloneSetupGenerated.
Just remove the return type void and everything behaves as expected ;-)

We keep the injector in a field to avoid that the KGraphIdeModule, which is registered in the KGraphIdeSetup is removed from the injector by someone who calls doSetup without ide modules in mind. I suggest to leave it this way to avoid future complications.

...e.cau.cs.kieler.klighd.incremental/META-INF/services/de.cau.cs.kieler.klighd.IUpdateStrategy

sailingKieler · 2020-09-09T16:11:29Z

plugins/de.cau.cs.kieler.klighd.standalone/META-INF/MANIFEST.MF

@@ -13,6 +13,7 @@ Require-Bundle: org.eclipse.emf.ecore.xmi,
 de.cau.cs.kieler.klighd.piccolo,
 org.eclipse.elk.alg.common;resolution:=optional,
 org.eclipse.elk.alg.force;resolution:=optional,
+ org.eclipse.elk.alg.graphviz.dot,


Is it really necessary that this dependency is non-optional?
Why is it required at all?

I will change it to being optional, I did not see that the others were optional as well when adding this dependency. It is required, as the LayoutMetaDataService called by the KlighdStandaloneSetup will load all ILayoutMetaDataProvider via ServiceLoader and will not be able to instanciate the GraphvizMetaDataProvider, as it has a dependency to graphviz.dot which will otherwise not be included.

sailingKieler · 2020-09-09T16:15:03Z

...s.kieler.klighd.standalone/src/de/cau/cs/kieler/klighd/standalone/KlighdStandaloneSetup.java

@@ -68,8 +68,6 @@ public void doInitialize() {
        );

        KlighdDataManager.getInstance()
-                .registerUpdateStrategy(SimpleUpdateStrategy.ID, new SimpleUpdateStrategy())
-                .registerViewer(PiccoloViewer.ID, new PiccoloViewer.Provider())


Is there any replacement for this deletion? AFAIR it is required for running unit tests in non-Eclipse mode.

Now that all update strategies and viewers can be registered via service loader and via extension point, registering these manually is not needed anymore. The simple update strategy is registered via extension point and service loader and will therefore be registered by the KlighdDataManager by default. The PiccoloViewer should not be registered here, as not all projects using the standalone setup (i.e., the KLighD Language Server) use it, it should not be registered therefore. I guess it should also be registered via extension point/service loader in the Piccolo Plugin therefore, however I don't see that happening there yet. I will look into that.

I'm with the changed registration of the SimpleUpdateStrategy, but I would advocate a registration of the (non-UI) PiccoloViewer.Provider in the klighd.piccolo.test project. Do you agree?

Currently, that Provider is registered in

KLighD/test/de.cau.cs.kieler.klighd.test/plugin.xml

Lines 6 to 9 in c5f6bb4

<viewer

class="de.cau.cs.kieler.klighd.piccolo.viewer.PiccoloViewer$Provider"

id="de.cau.cs.kieler.klighd.piccolo.viewer.PiccoloViewer">

</viewer>

, too, so having a corresponding service loader-based registration would be good.

Seems good to have it registered in the test plugins where it is needed instead of in the standalone setup. I will implement that tomorrow.

Done. The PiccoloViewer$Provider is now registered via Extension Point and via Service Loader in the klighd.test and klighd.piccolo.test plugins.

sailingKieler · 2020-09-09T16:18:12Z

plugins/de.cau.cs.kieler.klighd/plugin.xml

@@ -3,6 +3,7 @@
 <plugin>
   <extension-point id="extensions" name="Compound extension point for all kinds of extensions" schema="schema/extensions.exsd"/>
   <extension-point id="diagramSyntheses" name="Diagram syntheses registering point" schema="schema/diagramSyntheses.exsd"/>
+   <extension-point id="klighdStartupHook" name="Startup Hook for KLighD" schema="schema/klighdStartupHook.exsd"/>


We agreed on having the startupHooks as an extension type within the extensions point declared above, right?

Yes we did, and it is implemented as such. I guess I overlooked this reference when removing it again. Will remove.

.../de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/syntheses/AbstractDiagramSynthesis.java

sailingKieler · 2020-09-10T07:12:36Z

plugins/de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/syntheses/DiagramLayoutOptions.java

@@ -201,7 +201,7 @@ private DiagramLayoutOptions() {
    /**
     * @see CoreOptions#PORT_LABEL_PLACEMENT
     */
-    public static final IProperty<PortLabelPlacement> PORT_LABEL_PLACEMENT =
+    public static final IProperty<EnumSet<PortLabelPlacement>> PORT_LABEL_PLACEMENT =


That's an API breaking change, right? Did that change happen in ELK, too?

Has the corresponding change in ELK been released?

Yes, this was released in ELK 0.7.0

programmatic registration in the KlighdDataManager.

sailingKieler · 2020-09-16T07:25:52Z

I don't have time for a detailed review this week, but I guess you guys did the changes carefully.

Conflicts: plugins/de.cau.cs.kieler.klighd/META-INF/MANIFEST.MF

soerendomroes · 2020-09-18T10:01:06Z

plugins/de.cau.cs.kieler.klighd.standalone/pom.xml

@@ -16,7 +16,7 @@
  <dependencies>
    <dependency>
      <groupId>org.eclipse.elk</groupId>
-      <artifactId>org.eclipse.elk.core.service</artifactId>
+      <artifactId>org.eclipse.elk.core</artifactId>


Why do you remove elk.core.service here? Is it not needed?

Looking at the dependencies in the manifest, this dependency was wrong in the first place (the .service part is not required there, but the elk.core bundle directly). Furthermore, the only class in the package has only dependencies to elk.core and not elk.core.service, so this is correct like this.
Looking at the last build with this here also shows some issue in the maven build with this dependency, which is resolved with this change.

This change is wrong, as the class LightDiagramServices requires DiagramLayoutEngine from elk.service, see

KLighD/plugins/de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/LightDiagramServices.java

Lines 246 to 261 in cc19356

final DiagramLayoutEngine engine = new KlighdLayoutSetup().getDiagramLayoutEngine();

final IStatus status;

if (Klighd.IS_PLATFORM_RUNNING) {

final IElkCancelIndicator cancelationIndicator =

thePart != null ? new DispositionAwareCancelationHandle(thePart) : null;

status = engine.layout(thePart, diagramPart, cancelationIndicator, layoutParameters)

.getProperty(DiagramLayoutEngine.MAPPING_STATUS);

} else {

final IElkProgressMonitor progressMonitor = new NullElkProgressMonitor();

status = engine.layout(thePart, diagramPart, progressMonitor, layoutParameters)

.getProperty(DiagramLayoutEngine.MAPPING_STATUS);

}

For compiling elk.service is include via a Manifest dependency.
For this reason, elk.service is been published to maven central, see eclipse/elk#511
This, please re-add the dependency to elk.service, otherwise Klighd won't work in standalone mode "out-of-the-box".

Wrt.

Furthermore, the only class in the package has only dependencies to elk.core and not elk.core.service, so this is correct like this.

This project with its dependencies serves as a so-called Bill-Of-Materials (BOM), see https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#bill-of-materials-bom-poms

The aim is that only a dependency to this project is required in order to bring Klighd to work.

Okay, I did not know abot the BOM behavior of the project, so this makes sense as it was before. The initial reason why I changed this though is because the build suddenly broke as mentioned before and seen here and this at least fixed the Maven build again.
The issue in the build is summarized in the logs:

No versions are present in the repository for the artifact with a range [4.5.1,6.0.0) com.sun.jna:com.sun.jna:jar:null from the specified remote repositories: klighd (http://kieler.github.io/KLighD/repo, releases=true, snapshots=true), central (https://repo.maven.apache.org/maven2, releases=true, snapshots=false) Path to dependency: 1) de.cau.cs.kieler.klighd:de.cau.cs.kieler.klighd.standalone:pom:2.0.0.v20200918 2) org.eclipse.elk:org.eclipse.elk.core.service:jar:0.7.0 3) org.eclipse.platform:org.eclipse.ui.workbench:jar:3.110.0 4) org.eclipse.platform:org.eclipse.e4.ui.workbench.addons.swt:jar:1.3.1100 5) org.eclipse.platform:org.eclipse.e4.ui.workbench.renderers.swt:jar:0.14.1300 6) org.eclipse.platform:org.eclipse.e4.ui.workbench.swt:jar:0.14.1100 7) org.eclipse.platform:org.eclipse.urischeme:jar:1.1.100

As I now tracked this down, the Issue is only has nothing to do with the merge of keith and would have occured otherwise as well because of the org.eclipse.urischeme transitive dependency of elk.core.service, which had a new release 1.1.100 on Sept. 16th and introduced a new dependency to com.sun.jna version [4.5.1,6.0.0), which is not available on Maven Central. Only a previous version 3.0.5 is available there with a note to use net.java.dev.jna instead (see here).
As this new dependency is automatically resolved by maven, the build currently fails when re-adding the dependency to elk.core.service again.

I don't quite know enough about maven's own dependency management, as we otherwise only use the depenendy mechanism of tycho, so have not found a solution to this problem yet. Do you know how to resolve this now-broken transitive dependency? Sadly, this seems to be blocking the next KLighD release.

As this problem also affects the ELK nightly build, I submitted an Issue on the Eclipse bug tracker:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=567244

Do you know how to resolve this now-broken transitive dependency? Sadly, this seems to be blocking the next KLighD release.

Can we have a call on that today (Sept 23rd) afternoon?

ConnorObrian and others added 30 commits June 12, 2019 14:40

Constraints: Model is serialized back to the file

bff3ed3

Merge remote-tracking branch 'origin/jet_cos/constraints' into jet_co…

4981102

…s/constraints

reversed edges and width of nodes dont make problems anymore

9fb5bbb

klighd-lsp: updateLayout is now always called during diagram generation.

0489849

Merge branch 'keith' into jet_cos/constraints

485ef75

Constraints: Layout for constraints is now called in doUpdateLayout

f024080

Layout for Constraints is called in createModel

2063ec7

added comments

5839b7b

checking the interactiveLayout property

ec2d0bd

Constraints: Added layerID and positionID to KlighD for KGraphs

c88c686

initialLayers-method uses layerID of the nodes

bb45396

added props as attributes to SKNode

8aecd09

Constraints: Delete implemented into LSP + Comments

e6e7ae3

Merge remote-tracking branch 'origin/jet_cos/constraints' into jet_co…

7828a3a

…s/constraints

Constraints: Bugfix -LayerID,PosID are now correct after setting a cons

d9ef1c1

Merge branch 'master' into keith

5efdf10

Constraints: StaticConstraint created and first reevaluation methods

0717f80

added positionId to SKNode

8c5e1a8

Merge branch 'jet_cos/constraints' into jet_cos/Reevaluation

76111ff

Merge remote-tracking branch 'origin/master' into keith

9ce3f6a

SKNodes have interactiveLayout property

cc74bd9

Constraints: offsetPositionConstraints-method created

cef09dc

Merge branch 'jet_cos/constraints' into jet_cos/Reevaluation

361b189

Constraints: revaluation small improvements

2eb0711

Constraints: StaticConstraint class added

2911f1e

The last commit did not work without the StaticConstraint class that I forgot to commit

Merge branch 'jet_cos/constraints' into jet_cos/Reevaluation

91d30ba

Constraints: Implements setStaticConstraint. Fixes StaticConstraint.

ac39b6e

Merge branch 'jet_cos/constraints' into jet_cos/Reevaluation

4bc5e8b

Constraints: Improves after shift reevaluation

c10f869

NiklasRentzCAU and others added 14 commits August 18, 2020 10:19

klighd: default status manager also prints stack trace on error.

5ca065c

lsp: decode all URIs coming from the client first.

a50b86a

klighd.lsp: Added main thread queue for SWT and AWT calls.

3857c9c

lsp: Improved diagram handling for files with no Resource.

9edb76e

Instead of throwing exceptions, a message model displaying that no model is in the editor is shown.

all: merged the preservedProperties extension point into KLighD's

2773010

generic one.

Use Consumer instead of Function.

4beb7bf

Merge remote-tracking branch 'origin/keith' into mainThreadQueue Conflicts: plugins/de.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/KGraphLanguageServerExtension.xtend

klighd.lsp: Organized imports.

3ba6b74

Merge pull request #66 from kieler/mainThreadQueue

7596d70

klighd.lsp: Added main thread queue for SWT and AWT calls.

merged startup extension into KLighD's main extension.

588a149

Moved the initialization and loading of all extensions into the static part after creating the instance, so that the startupHook extension may use the instance and modify it directly.

Merge branch 'keith' of [email protected]:kieler/KLighD.git into keith

926dff3

klighd.lsp: Renamed interactive LS extensions #67.

2bef347

build: adapted to Xtext 2.22 and Sprotty 0.9

86657ea

lsp: handle exceptions in the main thread queue

2aaaf31

sailingKieler requested changes Sep 10, 2020

View reviewed changes

soerendomroes and others added 7 commits September 10, 2020 15:08

klighd: Corrected IGraphElementVisitor related javadoc and imports.

b451840

klghd: some code cleanup

c2b22c5

klighd.test, klighd.piccolo.test: registered PiccoloViewer.Provider

a86714e

klighd: Made IPreservedProperties extend Iterable and extended its

8988bee

programmatic registration in the KlighdDataManager.

Merge remote-tracking branch 'origin/master' into keith

4345e92

build: incremented tycho and xtext versions in maven build

742fa13

lsp: made resource loading thread-safe by synchronizing

4b61323

sailingKieler approved these changes Sep 16, 2020

View reviewed changes

NiklasRentzCAU added 3 commits September 18, 2020 10:54

Merge remote-tracking branch 'origin/master' into keith

1db75b0

Conflicts: plugins/de.cau.cs.kieler.klighd/META-INF/MANIFEST.MF

standalone: corrected imports

4822e95

piccolo.test: adapted to ELK 0.7.0 changes

1239195

soerendomroes commented Sep 18, 2020

View reviewed changes

NiklasRentzCAU merged commit cc19356 into master Sep 18, 2020

NiklasRentzCAU deleted the keith branch August 7, 2024 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Merge KEITH features #49

[WIP] Merge KEITH features #49

soerendomroes commented Jul 9, 2020 •

edited

Loading

sailingKieler left a comment

sailingKieler Sep 9, 2020

soerendomroes Sep 10, 2020

sailingKieler Sep 9, 2020

NiklasRentzCAU Sep 10, 2020

sailingKieler Sep 9, 2020

NiklasRentzCAU Sep 10, 2020

sailingKieler Sep 10, 2020

sailingKieler Sep 10, 2020

NiklasRentzCAU Sep 10, 2020

sailingKieler Sep 10, 2020

NiklasRentzCAU Sep 11, 2020

sailingKieler Sep 9, 2020

NiklasRentzCAU Sep 10, 2020

sailingKieler Sep 10, 2020

sailingKieler Sep 10, 2020

soerendomroes Sep 10, 2020

sailingKieler commented Sep 16, 2020

soerendomroes Sep 18, 2020

NiklasRentzCAU Sep 18, 2020

sailingKieler Sep 21, 2020

sailingKieler Sep 21, 2020 •

edited

Loading

NiklasRentzCAU Sep 21, 2020

NiklasRentzCAU Sep 23, 2020

sailingKieler Sep 23, 2020

	<viewer
	class="de.cau.cs.kieler.klighd.piccolo.viewer.PiccoloViewer$Provider"
	id="de.cau.cs.kieler.klighd.piccolo.viewer.PiccoloViewer">
	</viewer>

	final DiagramLayoutEngine engine = new KlighdLayoutSetup().getDiagramLayoutEngine();
	final IStatus status;

	if (Klighd.IS_PLATFORM_RUNNING) {
	final IElkCancelIndicator cancelationIndicator =
	thePart != null ? new DispositionAwareCancelationHandle(thePart) : null;

	status = engine.layout(thePart, diagramPart, cancelationIndicator, layoutParameters)
	.getProperty(DiagramLayoutEngine.MAPPING_STATUS);

	} else {
	final IElkProgressMonitor progressMonitor = new NullElkProgressMonitor();

	status = engine.layout(thePart, diagramPart, progressMonitor, layoutParameters)
	.getProperty(DiagramLayoutEngine.MAPPING_STATUS);
	}

[WIP] Merge KEITH features #49

[WIP] Merge KEITH features #49

Conversation

soerendomroes commented Jul 9, 2020 • edited Loading

sailingKieler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sailingKieler commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sailingKieler Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soerendomroes commented Jul 9, 2020 •

edited

Loading

sailingKieler Sep 21, 2020 •

edited

Loading