WIP: Feature/street map layout #905

lurichte · 2020-04-08T16:54:52Z

Integration of Street map layout

closes #904

Description

User is able to choose between 'Squarified Treemap' and 'Street Layout' in options which then redraws the map with the corresponding layout

CodeCharta 03/2020 with the following metrics:
Size: rloc, Height: mcc, Color: mcc

JUnit5 10/2019 with the following metrics:
Size: rloc, Height: mcc, Color: mcc

Definition of Done

A task/pull request will not be considered to be complete until all these items can be checked off.

Richargh · 2020-04-08T19:37:33Z

Do you have a screenshot for us? :D

…te/codecharta into feature/street-map-layout

feature/street-map-layout

alschmut · 2020-04-20T07:57:45Z

visualization/app/codeCharta/codeCharta.model.ts

+export enum LayoutAlgorithm {
+	SquarifiedTreeMap = "Squarified TreeMap",
+	StreetMap = "StreetMap",
+	TMStreet = "TMStreet"


This was the TreeMap Street, which combines Squarified and the Street right? I would then rather use some more descriptive name. Either TreeMapStreet or maybe you have more ideas?

alschmut · 2020-04-20T08:11:43Z

visualization/app/codeCharta/ui/maxTreeMapFiles/maxTreeMapFiles.component.spec.ts

+import { maxTreeMapFiles } from "../../state/store/appSettings/maxTreeMapFiles/maxTreeMapFiles.reducer"
+import _ from "lodash"
+
+describe("MaxTreeMapFilesController", () => {


Add tests for the methods onMaxTreeMapFilesChanged() and onChangeMaxTreeMapFilesSlider() as well as the constructor setting the maxTreeMapFiles. You can have a look at the layoutSelection.component.spec.ts, which is quite similar

alschmut · 2020-04-20T08:15:24Z

visualization/app/codeCharta/ui/maxTreeMapFiles/maxTreeMapFiles.component.ts

+		const maxTreeMapFiles = this.storeService.getState().appSettings.maxTreeMapFiles
+		this.onMaxTreeMapFilesChanged(maxTreeMapFiles)
+		this._viewModel.maxTreeMapFiles = maxTreeMapFiles


Can you extract that in a function called initOnOpenDialog() or similar? Otherwise somebody might remove the lines, as it looks like a duplicate for onMaxTreeMapFilesChanged(), but it is needed as a closed (non existing) dialog, does not receive events.

And remove line 18, as this one is a duplicate of the function call in onMaxTreeMapFilesChanged() :)

alschmut · 2020-04-20T08:17:43Z

visualization/app/codeCharta/util/__snapshots__/layoutHelper.spec.ts.snap

+  "depth": 0,
  "edgeAttributes": undefined,
  "flat": false,
  "height": 100,
  "heightDelta": 33,
  "incomingEdgePoint": Vector3 {
    "x": 199,
-    "y": 102,
+    "y": 100,
    "z": 99,


Why are there some coordinates changing by 1 or 2? Same for the rest of the file

alschmut · 2020-04-20T08:46:26Z

visualization/app/codeCharta/util/layoutHelper.ts

+export interface LayoutNode {
+	data: CodeMapNode
+	value: number
+	rect: Rectangle
+	zOffset: number
+}


As this is our own generic interface, I would rename some of the properties to be more readable

export interface LayoutNode { node: CodeMapNode areaValue: number rectangle: Rectangle zOffset: number }

alschmut · 2020-04-20T10:08:22Z

visualization/app/codeCharta/util/layoutHelper.ts

+		return !node.children || node.children.length === 0
+	}
+
+	public static mergeDirectories(node: CodeMapNode, metricName: string): CodeMapNode {


Can you shortly explain what this method does and why it is needed?

alschmut · 2020-04-20T10:11:56Z

visualization/app/codeCharta/util/streetLayoutGenerator.ts

+export interface StreetLayoutValuedCodeMapNode {
+	data: CodeMapNode
+	value: number
+	rect: Rectangle
+	zOffset: number
+}


Lets remove this interface and use the other LayoutNode which is the same, since we refactored some code

After having a deeper look at the code I have a better idea. Let's:

keep the interface, but rename it to StreetLayoutNode (its still good enough isn't it?)

remove its properties and extend the LayoutNode (yes, those interface are then the same for now, but make future additions very easy to apply)

That means, we can have different interfaces for each individual layoutAlgorithm, but when building the Node[] inside the LayoutHelper, we always cast/map those to a LayoutNode

alschmut · 2020-04-20T10:23:09Z

visualization/app/codeCharta/util/streetLayoutGenerator.ts

+		})
+	}
+
+	private static createBoxes(


Can you refactor that method to be shorter and/or split into multiple more readable methods?

alschmut · 2020-04-20T10:30:22Z

visualization/app/codeCharta/util/layoutHelper.ts

+	public static calculateSize(node: CodeMapNode, metricName: string) {
+		let totalSize = node.attributes[metricName] || 0
+
+		if (totalSize === 0 && node.children && node.children.length > 0) {
+			for (const child of node.children) {
+				totalSize += LayoutHelper.calculateSize(child, metricName)
+			}
+		}
+		return totalSize
+	}


Are you aware of that for folder nodes, the sum of its children is set inside the NodeDecorator.decorateParentNodesWithAggregatedAttributes())? The nodes contain that sum/median depending on the attributeType.

alschmut

As already said, combining the StreetMapLayout with CodeCharta was probably already a wish since the TestVille project was forked many years ago. So we really appreciate your hard work on your self written algorithms 🚀🥳 Overall the code looks really good! Just got some questions and comments. No need to be worried ✌️

Important! I didn't have a close look into the actual algorithm, so maybe somebody else would like to do that?

Inside the algorithm classes there are quite a lot of comments. Some are almost duplicates of the function name, some give a bit more information. I personally prefer accurate method names than having a bunch of comments. Maybe you can remove some comments or replace those by a more specific method name. @NearW what do you think?

And finally, as the algorithm classes contain quite some logic, those should also be unit tested (with human sense) like for every other class in the project.

And like always, whenever you have questions, just ask ;)

@BridgeAR

* initial street layout introduction * may be a fix for e2e filechooser test * street layout flattening fix, filechooser e2e fix, global setting dialog layout improvement, code refactoring * fixed TMStreet layout * added max treemap files * fixing linting, added remark about e2e falky test fix, added streetlayout todo * edited changelog * fixes after @BridgeAR review * updated changelog * Apply code formating #1543 * Update visualization/app/codeCharta/ui/maxTreeMapFiles/maxTreeMapFiles.component.ts * Update visualization/app/codeCharta/util/algorithm/streetLayout/horizontalStreet.ts * Update visualization/app/codeCharta/streetLayoutTodo.md * Update visualization/app/codeCharta/util/algorithm/streetLayout/streetViewHelper.ts * Update visualization/app/codeCharta/util/algorithm/streetLayout/verticalStreet.ts * Update visualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapHelper.ts * Improve identation #905 * Merge main, Fix label position conflicts #905 * Fix scaling bug and improve performance * Fix label positioning bug on mouse move, Apply minor refactorings. * Add regression tests for label scaling/positioning bugs * Fix temporary label bug: reset on map move/turn Co-authored-by: Cedrik Bormann <cedrikbormann@gmail.com> Co-authored-by: Cedrik Bormann <26900540+ce-bo@users.noreply.github.com>

BridgeAR · 2020-12-24T16:45:08Z

Closing as it is superseded by another PR that was merged by now. @lurichte thank you very much for all the work!

@BridgeAR

* initial street layout introduction * may be a fix for e2e filechooser test * street layout flattening fix, filechooser e2e fix, global setting dialog layout improvement, code refactoring * fixed TMStreet layout * added max treemap files * fixing linting, added remark about e2e falky test fix, added streetlayout todo * edited changelog * fixes after @BridgeAR review * updated changelog * Apply code formating #1543 * Update visualization/app/codeCharta/ui/maxTreeMapFiles/maxTreeMapFiles.component.ts * Update visualization/app/codeCharta/util/algorithm/streetLayout/horizontalStreet.ts * Update visualization/app/codeCharta/streetLayoutTodo.md * Update visualization/app/codeCharta/util/algorithm/streetLayout/streetViewHelper.ts * Update visualization/app/codeCharta/util/algorithm/streetLayout/verticalStreet.ts * Update visualization/app/codeCharta/util/algorithm/treeMapLayout/treeMapHelper.ts * Improve identation #905 * Merge main, Fix label position conflicts #905 * Fix scaling bug and improve performance * adding layout reset behavior * Fix hard coded year in unit test * fix #1624 * fix #1624 * fix test * fix test * fix test * fix flaky test * fix flaky test * fix flaky Integration test * added agenda regression tests for 53 week years * Update CalendarWeekTest.kt * Update CalendarWeekTest.kt * Update CalendarWeekTest.kt * possible fix for flaky test timeout * added some comments & updated changelog * remove trailing spaces Co-authored-by: Cedrik Bormann <cedrikbormann@gmail.com> Co-authored-by: Cedrik Bormann <26900540+ce-bo@users.noreply.github.com> Co-authored-by: Torsten Knauf <Torsten.Knauf@maibornwolff.de>

Lukas Richter and others added 13 commits April 7, 2020 15:20

layoutSelection component added

4cda68d

added layoutSelection component

f035edd

create layoutAlgorithm property in redux store

de23b36

Connect Layout Algorithm with redux

d07c27a

Add Layout differenciation

95a6b87

Add StreetMap algorithm

45c155b

Move layout selection to global settings

c0035ba

Add custom Squarified Treemap for TMStreet-Layout

b958c55

Fix typo

e43ad65

Add margin to treemap and height calculation

1e7dda7

Refactor use of streetLayoutHelper and use treeMapHelper instead

c8f8997

Restructure algorithm directories

d9de094

Fix imports

0cdc88e

lurichte and others added 13 commits April 9, 2020 12:58

Add check if node has children

155e9c4

Add check if node has children

8c59857

Add TMStreet Layout

531085e

Fix data structure and names

af57496

Merge branch 'master' into feature/street-map-layout

1b93b61

Remove unneeded imports MaibornWolff#904

5c58a0b

Add TreeMapStartDepth option for TMStreet Layout

7614690

Merge branch 'feature/street-map-layout' of https://github.com/lurich…

3074837

…te/codecharta into feature/street-map-layout

Fix layout selection test file MaibornWolff#904

31c14af

Add tests for layoutSelection.component MaibornWolff#904

2503bf8

Fix test in layoutSelection.component MaibornWolff#904

31e232b

Add TreeMapStartDepth ui to global settings MaibornWolff#904

59b1e48

Merge branch 'feature/street-map-layout' of https://github.com/lurich…

4f81fa0

…te/codecharta into feature/street-map-layout

alschmut added the pr-visualization Issues that touch the visualization pr(oject) which means web and desktop features. label Apr 10, 2020

lurichte added 2 commits April 14, 2020 12:21

Change treemapDepth slider to number picker MaibornWolff#904

8a8abf8

Fix LayoutSelection spec file

287a630

lurichte and others added 5 commits April 17, 2020 11:41

Remove obsolete snapshot file MaibornWolff#904

691d8a4

Merge branch 'master' into

dc7c4c8

feature/street-map-layout

Merge branch 'master' into feature/street-map-layout

dba8e2d

Merge branch 'master' into feature/street-map-layout

2124d5d

Merge branch 'master' into feature/street-map-layout

bdb1861

alschmut reviewed Apr 20, 2020

View reviewed changes

alschmut self-assigned this Apr 23, 2020

CodeCharta changed the title ~~Feature/street map layout~~ WIP: Feature/street map layout Apr 24, 2020

NearW marked this pull request as draft July 28, 2020 00:35

NearW changed the base branch from master to main August 3, 2020 08:27

Med-H self-assigned this Dec 10, 2020

Med-H added this to In progress in CC via automation Dec 10, 2020

Med-H mentioned this pull request Dec 14, 2020

Feature/905/street map layout #1585

Merged

8 tasks

ce-bo added a commit that referenced this pull request Dec 21, 2020

Improve identation #905

d6ef2db

ce-bo added a commit that referenced this pull request Dec 21, 2020

Merge main, Fix label position conflicts #905

732235f

BridgeAR closed this Dec 24, 2020

CC automation moved this from In progress to Done Dec 24, 2020

Med-H pushed a commit that referenced this pull request Jan 18, 2021

Merge main, Fix label position conflicts #905

3acfbd4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Feature/street map layout #905

WIP: Feature/street map layout #905

lurichte commented Apr 8, 2020 •

edited

Richargh commented Apr 8, 2020

alschmut Apr 20, 2020

alschmut Apr 20, 2020 •

edited

alschmut Apr 20, 2020

alschmut Apr 20, 2020 •

edited

alschmut Apr 20, 2020

alschmut Apr 20, 2020

alschmut Apr 20, 2020

alschmut Apr 20, 2020 •

edited

alschmut Apr 20, 2020

alschmut Apr 20, 2020

alschmut left a comment •

edited

BridgeAR commented Dec 24, 2020

WIP: Feature/street map layout #905

WIP: Feature/street map layout #905

Conversation

lurichte commented Apr 8, 2020 • edited

Integration of Street map layout

Description

Definition of Done

Richargh commented Apr 8, 2020

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020 • edited

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020 • edited

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020 • edited

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut Apr 20, 2020

Choose a reason for hiding this comment

alschmut left a comment • edited

Choose a reason for hiding this comment

BridgeAR commented Dec 24, 2020

lurichte commented Apr 8, 2020 •

edited

alschmut Apr 20, 2020 •

edited

alschmut Apr 20, 2020 •

edited

alschmut Apr 20, 2020 •

edited

alschmut left a comment •

edited