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

Bug/1095/number of edges not visible #1111

Merged
merged 82 commits into from
Aug 26, 2020

Conversation

NearW
Copy link
Contributor

@NearW NearW commented Jul 24, 2020

Fix number of edges not visible when hovering

closes #1095

Description

The bug existed, because we already started rendering when the metricData was set and didn't await the edgeMetricData. I decided that instead of adding a simple if-condition to ensure that, we should rework some stuff and finally add the metrics to the redux store. This was a huge task. Some changes are trivial, but especially the nodeMetricService/Reducer and edgeMetricService/Reducer are not.

  • Removed metricService and edgeMetricDataService and created a nodeMetricData and edgeMetricData property in the redux store instead
  • Moved all logic that involves calculating and setting the state to it's reducer (you can now set the state by dispatching an action)
  • Moved all logic that involves retrieving information from the state to the corresponding service
  • Created two new types NodeMetricData and EdgeMetricData and used the previous MetricData for the parent-reducer
  • Created a new event metric-data-complete that is triggered, when both node and edge metric data is calculated
  • Fixed bug by listening to that event and rendering when all the metric data is available
  • Removed all occurrences of jest.resetAllMocks() since they're not doing anything anyway
  • Changed edge-metrics being sorted by name now as we do with the node-metrics instead of by number of incoming and outgoing edges

In order to review this, check out the two new services I created and check every file that just required small changes (such as fixing the import and injecting the new service). I can help you with the rest then. :)

ToDo

  • Add regression test

Definition of Done

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

  • All requirements mentioned in the issue are implemented
  • Does match the Code of Conduct and the Contribution file
  • Task has its own GitHub issue (something it is solving)
    • Issue number is included in the commit messages for traceability
  • Update the README.md with any changes/additions made
  • Update the CHANGELOG.md with any changes/additions made
  • Enough test coverage to ensure that uncovered changes do not break functionality
  • All tests pass
  • Descriptive pull request text, answering:
    • What problem/issue are you fixing?
    • What does this PR implement and how?
  • Assign your PR to someone for a code review
    • This person will be contacted first if a bug is introduced into master
  • Manual testing did not fail

NearW added 8 commits July 23, 2020 23:57
…mber-of-edges-not-visible

# Conflicts:
#	visualization/app/codeCharta/state/metric.service.ts
#	visualization/app/codeCharta/ui/edgeChooser/edgeChooser.component.spec.ts
#	visualization/app/codeCharta/ui/edgeSettingsPanel/edgeSettingsPanel.component.spec.ts
#	visualization/app/codeCharta/ui/mapTreeView/mapTreeView.level.component.spec.ts
#	visualization/app/codeCharta/util/dataMocks.ts
#	visualization/app/codeCharta/util/fileDownloader.ts
#	visualization/app/codeCharta/util/scenarioHelper.ts
#	visualization/package-lock.json
@NearW NearW added bug Only issues that describe bugs. pr-visualization Issues that touch the visualization pr(oject) which means web and desktop features. tech For technical stories without user impact (=refactoring stories). labels Jul 24, 2020
@BridgeAR
Copy link
Member

Needs a rebase

NearW and others added 2 commits July 31, 2020 20:15
…mber-of-edges-not-visible

# Conflicts:
#	visualization/app/codeCharta/ui/edgeChooser/edgeChooser.e2e.ts
CHANGELOG.md Outdated Show resolved Hide resolved
@NearW NearW marked this pull request as draft August 3, 2020 07:24
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thanks for following up on the comments! This is shaping nicely!

return this.storeService.getState().metricData.nodeMetricData.some(x => x.name == metricName)
}

public getMaxMetricByMetricName(metricName: string): number {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public getMaxMetricByMetricName(metricName: string): number {
public getMaxMetricByMetricName(metricName: string): number | undefined {

I could imagine that metric is always truthy due to just accepting known metricNames (I suggest to use an enum to guarantee that metricName is always known).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we only call this method on metrics we know exist.

How can we build an enum dynamically containing only the metrics that are relevant to the visible files though?

@NearW
Copy link
Contributor Author

NearW commented Aug 25, 2020

@BridgeAR Great suggestions. I decided that we should sort the edge-metrics the same way, we sort the node-metrics (by name). This way, I was able to resolve the duplicate of the sort-function.

@BridgeAR
Copy link
Member

@NearW do you have a before and after screenshot for the change? :-)

@NearW
Copy link
Contributor Author

NearW commented Aug 26, 2020

Before:
image

After:
image

Edge-Metrics will be sorted the same way, we sort the node-metrics: By name. Before they were sorted by the max-value of incoming and outgoing edges. Sorting them by max-value isn't a bad idea, but we should handle both metrics the same way in my opinion.

I also pushed another huge performance improvement for huge maps.

@BridgeAR BridgeAR merged commit cc5ddca into main Aug 26, 2020
@BridgeAR BridgeAR deleted the bug/1095/number-of-edges-not-visible branch August 26, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Only issues that describe bugs. pr-visualization Issues that touch the visualization pr(oject) which means web and desktop features. tech For technical stories without user impact (=refactoring stories).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Number of incoming and outgoing edges not visible
3 participants