-
Notifications
You must be signed in to change notification settings - Fork 5
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
Master Thesis - SCChart Edge Merge - Code Review #7
Draft
Skgland
wants to merge
5
commits into
kieler:master
Choose a base branch
from
Skgland:ben/edge_group_clean
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
56a0006
fix spelling and make option text clearer
Skgland f26a735
add EdgeMergeHook
Skgland e830dd8
add comments for the edge merge hook
Skgland a4ce7ed
undo display name change of hide annotation synthesis hook
Skgland 23071bf
add a doc comment to the class
Skgland File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletions
1
....sccharts.ui/META-INF/services/de.cau.cs.kieler.sccharts.ui.synthesis.hooks.SynthesisHook
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
115 changes: 115 additions & 0 deletions
115
...s.kieler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/hooks/EdgeMergeHook.xtend
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
/* | ||
* KIELER - Kiel Integrated Environment for Layout Eclipse RichClient | ||
* | ||
* http://www.informatik.uni-kiel.de/rtsys/kieler/ | ||
* | ||
* Copyright 2021,2022 by | ||
* + Kiel University | ||
* + Department of Computer Science | ||
* + Real-Time and Embedded Systems Group | ||
* | ||
* This code is provided under the terms of the Eclipse Public License (EPL). | ||
* See the file epl-v10.html for the license text. | ||
*/ | ||
package de.cau.cs.kieler.sccharts.ui.synthesis.hooks | ||
|
||
import de.cau.cs.kieler.klighd.krendering.ViewSynthesisShared | ||
import de.cau.cs.kieler.sccharts.ui.synthesis.hooks.SynthesisHook | ||
import de.cau.cs.kieler.klighd.kgraph.KNode | ||
import de.cau.cs.kieler.sccharts.State | ||
import de.cau.cs.kieler.klighd.kgraph.KEdge | ||
import de.cau.cs.kieler.sccharts.Transition | ||
import de.cau.cs.kieler.sccharts.Scope | ||
import org.eclipse.xtend.lib.annotations.Data | ||
import de.cau.cs.kieler.sccharts.PreemptionType | ||
import de.cau.cs.kieler.sccharts.HistoryType | ||
import de.cau.cs.kieler.sccharts.DeferredType | ||
import java.util.HashMap | ||
import de.cau.cs.kieler.sccharts.ui.synthesis.GeneralSynthesisOptions | ||
import de.cau.cs.kieler.klighd.SynthesisOption | ||
import de.cau.cs.kieler.sccharts.DelayType | ||
|
||
/** | ||
* Merge transitions that are considered semantically similar to be visualized using one edge. | ||
*/ | ||
@ViewSynthesisShared | ||
class EdgeMergeHook extends SynthesisHook { | ||
|
||
|
||
/** The related synthesis option */ | ||
public static final SynthesisOption MERGE_SIMILAR_EDGES = SynthesisOption.createCheckOption(EdgeMergeHook, "Merge Similar Edges", | ||
false).setCategory(GeneralSynthesisOptions::APPEARANCE); | ||
|
||
override getDisplayedSynthesisOptions() { | ||
return newLinkedList(de.cau.cs.kieler.sccharts.ui.synthesis.hooks.EdgeMergeHook.MERGE_SIMILAR_EDGES) | ||
} | ||
|
||
/** | ||
* a mapping of an edge group key to the representative edge for that group | ||
*/ | ||
HashMap<EdgeGroupKey, KEdge> map = new HashMap<EdgeGroupKey,KEdge>(); | ||
|
||
/** | ||
* A Key used to determine if two transitions are semantically similar to be represented by one edge | ||
*/ | ||
@Data | ||
static class EdgeGroupKey { | ||
State source; | ||
State target; | ||
PreemptionType preemption; | ||
HistoryType history; | ||
DeferredType deferred; | ||
DelayType delay; | ||
boolean deterministic; | ||
} | ||
|
||
/** | ||
* Invoked before the translation of the model starts. | ||
* @param scopethe input model | ||
* @param nodethe empty diagram root node | ||
*/ | ||
override void start(Scope scope, KNode node) { | ||
// reset the map of representative edges as we start a new synthesis | ||
map.clear() | ||
} | ||
|
||
/** | ||
* Invoked after a {@link Transition} is translated. | ||
* @param transitionthe transition | ||
* @param edgethe translated edge | ||
*/ | ||
override void processTransition(Transition transition, KEdge edge) { | ||
|
||
if (MERGE_SIMILAR_EDGES.booleanValue) { | ||
|
||
// build the transitions key to find the representing edge | ||
// or insert the current edge if no edge was already chosen | ||
var key = new EdgeGroupKey( | ||
transition.sourceState, | ||
transition.targetState, | ||
transition.preemption, | ||
transition.history, | ||
transition.deferred, | ||
transition.delay, | ||
transition.isNondeterministic | ||
); | ||
|
||
// if we do not already have a representative use the current edge as the representative | ||
val rep = map.computeIfAbsent(key, [EdgeGroupKey _key | edge]); | ||
|
||
if (edge != rep) { | ||
// if this was not just chosen as the representative transfer all edge labels to the representative | ||
rep.labels.addAll(edge.labels) | ||
for (label : edge.labels) { | ||
// adjust the transfered labels parent | ||
label.parent = rep | ||
} | ||
// remove the current edge from the graph, as it will be represented by the representative edge | ||
edge.source.outgoingEdges.remove(edge) | ||
edge.target.outgoingEdges.remove(edge) | ||
} | ||
|
||
} | ||
} | ||
|
||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation on what this hook does and comments about how is accomplished in the code help understand in this file (other than the methods that are overriden and are by that kind of documented already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments, anything that is still missing sufficient explanation?
Should I remove javadoc comments from the overridden functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, anything that is API of new code should be documented so that someone not knowing what some class/method is about gets to know it by reading the JavaDoc for it. Ideally as you did here also with some comments sprinkled inbetween for calculations you do that are not immedeately obvious.
The only thing missing for me would now be a JavaDoc on the class itself explaining what this hook does.
The JavaDoc for overridden functions are imo not necessary, but they do not hurt either, so I would let them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now added a doc comment for the class.
I attempted to keep it similar detailed to other hooks' class comment.
I believe this was the only outstanding review comment from you and should therefore be ready for @a-sr to review.