Code Review: tik/proxy-view #54
Conversation
....cs.kieler.language.server/run-configurations/LanguageServer with KLighD in Workspace.launch
Outdated
Show resolved
Hide resolved
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Show resolved
Hide resolved
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Outdated
Show resolved
Hide resolved
| // Set size to be square and at least 34 (same as minimal node size) | ||
| val proxyBounds = PlacementUtil.estimateSize(proxy) | ||
| val minSize = 34 | ||
| val bigEnough = proxyBounds.width > 10 && proxyBounds.height > 10 |
There was a problem hiding this comment.
where do these values come from, how are they chosen and what do they control?
There was a problem hiding this comment.
I chose those values myself and tested out when the original proxy bounds would be appropriately large enough. It's needed in case there's no text or the text doesn't exceed the chosen threshold size - as the proxy might be barely visible in these cases. Doing this allows for non-square proxies
Do you have a suggestion on how to improve this or where to add what documentation?
There was a problem hiding this comment.
at least extract it into a variable with a little documentation explaining the choice that way changing the square size later is also trivial
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Outdated
Show resolved
Hide resolved
...rts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/filtering/SCChartsSemanticFilterRules.java
Outdated
Show resolved
Hide resolved
...arts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/filtering/SCChartsSemanticFilterTags.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public class SCChartsSemanticFilterRules { | ||
| // Naming conventions: | ||
| // The names should start with NO_ or ONLY_, indicating |
There was a problem hiding this comment.
also as discussed, use a naming convention that utilizes the checkbox as the "verb"
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Outdated
Show resolved
Hide resolved
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Outdated
Show resolved
Hide resolved
| ) | ||
| node.setProperty(KlighdProperties.SEMANTIC_FILTER_TAGS, semanticTags) | ||
| val proxy = createNode().associateWith(region) | ||
| val maxProxyLabelLength = 5 |
There was a problem hiding this comment.
avoid magic numbers. Why 5?
There was a problem hiding this comment.
5 is a good compromise of cutting as the proxies aren't too wide and usually 5 characters suffice for uniquely identifying the corresponding node
Also fun fact: in English, the average word length is about 5 or less https://www.researchgate.net/figure/Average-word-length-in-the-English-language-Different-colours-indicate-the-results-for_fig1_230764201
...eler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/ControlflowRegionSynthesis.xtend
Show resolved
Hide resolved
....kieler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/DataflowRegionSynthesis.xtend
Outdated
Show resolved
Hide resolved
...cau.cs.kieler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/SCChartsSynthesis.xtend
Outdated
Show resolved
Hide resolved
...de.cau.cs.kieler.sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/StateSynthesis.xtend
Outdated
Show resolved
Hide resolved
...rts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/filtering/SCChartsSemanticFilterRules.java
Outdated
Show resolved
Hide resolved
....sccharts.ui/src/de/cau/cs/kieler/sccharts/ui/synthesis/styles/ControlflowRegionStyles.xtend
Show resolved
Hide resolved
| if (grid instanceof KGridPlacement) { | ||
| grid.numColumns = grid.numColumns + 1 | ||
| addRectangle => [ | ||
| setGridPlacementData(5,5) |
There was a problem hiding this comment.
more magic numbers. Do we have a 5x5 grid here?
There was a problem hiding this comment.
Same as in the above method. In general, the code is mostly copied "as is done in synthesis-world"
|
This PR had some nasty revert commit, the linked new PR has the same changes with commits after the revert cherry-picked to avoid this issue. Closing, but keeping this PR in mind for the comments. |
Dependency on kieler/KLighD#129