Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrated eclipse.ui/SWT dependencies. #762

Closed

Conversation

soerendomroes
Copy link
Contributor

@soerendomroes soerendomroes commented Jun 21, 2021

The core.service plugin bundles functionality that is currently Eclipse dependent, but could be used outside of an Eclipse context (such as the DiagramLayoutEngine). I suggest to split the core.service plugin in an Eclipse dependent and independent part.

Ideally, we would want to do this also for downstream projects. For Klighd we decided it would be best to mock SWT und eclipse.ui instead via package dependencies and a mock plugin. The other alternative to deeclipsify Klighd would be to do something similar to the changes here and split every synthesis into three parts, which is why we don't want to do this now and try to mock it instead.

The DiagramLayoutEngine is currently used for interactive layout using a language server (or KIELER). However, we do not need the eclipse dependencies in that case. Moreover, we are working on a general diagram server/VSCode plugin and others to be able to use this too without understanding our whole build infrastructure. Currently, we have to supply three language servers for this. With an eclipse independent ELK (and Klighd) this does not have to be the case.

tldr: We need the not-eclipse-dependent functions and currently have dependencies to eclipse, which we start to elimate beginning with ELK.

Signed-off-by: Soeren Domroes sdo@informatik.uni-kiel.de

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@soerendomroes soerendomroes marked this pull request as ready for review June 23, 2021 14:04
@soerendomroes soerendomroes requested review from uruuru and sailingKieler and removed request for uruuru June 23, 2021 14:04
@soerendomroes soerendomroes force-pushed the sdo/Deeclipsification branch from 6df1e1f to bf92f96 Compare June 25, 2021 07:19
Conflicts:
	build/de.cau.cs.kieler.semantics.targetplatform/de.cau.cs.kieler.semantics.targetplatform.target
	plugins/de.cau.cs.kieler.kicool.ui/META-INF/MANIFEST.MF

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@sailingKieler
Copy link
Contributor

General question: What is the motivation for/benefit of these changes?
Or other words: What are use cases that cannot be solved with the existing architecture?

Please add @spoenemann as reviewers, as he designed the current state.
What about @le-cds?

@soerendomroes
Copy link
Contributor Author

soerendomroes commented Jun 28, 2021

General question: What is the motivation for/benefit of these changes?
Or other words: What are use cases that cannot be solved with the existing architecture?

@sailingKieler I added a short description of the use case to the PR description.

@soerendomroes soerendomroes self-assigned this Jun 29, 2021
Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@spoenemann
Copy link
Member

I have not used the Eclipse-specific integration in the last five years or so. I would be fine with dropping that part completely if it's too much effort to maintain it. We should rather reduce ELK to its core value, a layout library for Java, than complicating things too much. But feel free to do the splitting as you proposed 👍

@soerendomroes
Copy link
Contributor Author

An alternative to this approach would be to migrate Eclipse independent more general classes in an own plugin instead of more eclipse dependent in a new one.
Pros:

  • Interface remains the same for Eclipse users
    Cons:
  • service.core plugin moves into a new feature

Remove Java 8 since it was also done for elkjs.

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
Copy link
Contributor

@uruuru uruuru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same opinion as Miro.

Signed-off-by: Soeren Domroes <sdo@informatik.uni-kiel.de>
@soerendomroes
Copy link
Contributor Author

@le-cds If you have nothing to add I am willing to merge this

@soerendomroes soerendomroes marked this pull request as draft November 18, 2021 16:02
@soerendomroes
Copy link
Contributor Author

I think this is no longer necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants