Mka/elk properties #110
Mka/elk properties #110
Conversation
...e.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/DefaultBlacklistedProperties.java
Outdated
Show resolved
Hide resolved
...e.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/DefaultBlacklistedProperties.java
Outdated
Show resolved
Hide resolved
...e.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/DefaultBlacklistedProperties.java
Outdated
Show resolved
Hide resolved
| @@ -98,6 +98,9 @@ | |||
|
|
|||
| /** name of the 'preservedProperties' element. */ | |||
| private static final String PRESERVED_PROPERTIES = "preservedProperties"; | |||
There was a problem hiding this comment.
I think the preserved properties are no longer in use now, correct?
There was a problem hiding this comment.
They're still used for copying some stuff from the elkgraph to the KGraph apparently.
There was a problem hiding this comment.
No, I think you removed the only reference to it by introducing the blacklist and copying all other properties such as desiredPostion, layerId, etc...
There was a problem hiding this comment.
Again, is this still needed?
There was a problem hiding this comment.
Currently the only remaining reference is in the KGraphMerger. It's used there in conjunction with some property removal process to filter which properties should be copied from an elkgraph to a kgraph. This seems to be a separate thing from where preservedProperties have now been removed from and I'm not sure what the reasoning behind the filtering there is. To me it looks like preservedProperties were overloaded in their purpose before. I don't know what the right way to change KGraphMerger is. If that is solved, then yes preservedProperties and the related classes can be removed.
...e.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/DefaultBlacklistedProperties.java
Outdated
Show resolved
Hide resolved
| var properties = kNode.allProperties; | ||
| var propertyKeys = properties.keySet; | ||
| var blackList = KlighdDataManager.instance.blacklistedProperties; | ||
| for (key : propertyKeys) { |
There was a problem hiding this comment.
For better efficiency here you could just run through the properties instead of only the keys. Removes the need to search through the properties via get(key) for every key
| @@ -237,6 +240,9 @@ public static KlighdDataManager getInstance() { | |||
|
|
|||
| /** the properties that shall be preserved from the layout graph to the kgraph */ | |||
| private final List<IProperty<?>> preservedProperties = Lists.newArrayList(); | |||
|
|
|||
| /** the properties that are not allowed to be preserved */ | |||
There was a problem hiding this comment.
Preserved for what? The context is missing here in this generic class
| @@ -670,6 +684,18 @@ public KlighdDataManager registerPreservedProperties(Iterable<IProperty<?>> pres | |||
| } | |||
| return this; | |||
| } | |||
|
|
|||
| public KlighdDataManager registerBlacklistedProperty(IProperty<?> blacklistedProperty) { | |||
|
|
||
|
|
||
| /** | ||
| * Returns the list of registered properties that have been blacklisted. |
There was a problem hiding this comment.
Blacklisted for what? The context is missing here in this generic class
…culatedDecorationMap for now)
...ns/de.cau.cs.kieler.klighd.lsp/src/de/cau/cs/kieler/klighd/lsp/utils/KGraphMappingUtil.xtend
Outdated
Show resolved
Hide resolved
|
|
||
| for (propertyKVPair : properties.entrySet()) { | ||
| if (!containsPropertyWithId(blackList, propertyKVPair.key.id)) { | ||
| // TODO: remove this check once https://github.com/kieler/semantics/pull/13 has been merged |
There was a problem hiding this comment.
I guess we want to remove this before the next release and not after this was merged. (also see cases below)
There was a problem hiding this comment.
Also in EObjectSerializer
| @@ -77,5 +77,12 @@ | |||
| </provider> | |||
|
|
|||
| </extension> | |||
| <extension | |||
There was a problem hiding this comment.
Do we still need extension points @NiklasRentzCAU ?
There was a problem hiding this comment.
Yes. If something will be written to this setup (for example actions, syntheses etc. like in semantics) it will only be executed in Eclipse when used as an extension point. That was the main reason why we have this single programmatic extension mechanism now, that needs to be registered once as a Java Service and Eclipse extension point
| @@ -98,6 +98,9 @@ | |||
|
|
|||
| /** name of the 'preservedProperties' element. */ | |||
| private static final String PRESERVED_PROPERTIES = "preservedProperties"; | |||
There was a problem hiding this comment.
Again, is this still needed?
plugins/de.cau.cs.kieler.klighd/src/de/cau/cs/kieler/klighd/KlighdDataManager.java
Outdated
Show resolved
Hide resolved
Server-side changes to support changes proposed in klighd-vscode.
Additionally changes in semantics are necessary.