Fix/901/excluding last building error #1152
Conversation
NearW
added
bug
…ding-last-building-error # Conflicts: # CHANGELOG.md
…ding-last-building-error # Conflicts: # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.ts # visualization/app/codeCharta/ui/nodeContextMenu/nodeContextMenu.component.spec.ts # visualization/app/codeCharta/ui/nodeContextMenu/nodeContextMenu.component.ts
| if ( | ||
| !NodeDecorator.doesExclusionResultInEmptyMap( | ||
| this.codeMapPreRenderService.getRenderMap(), | ||
| this.storeService.getState().fileSettings.blacklist | ||
| ) | ||
| ) { | ||
| this.storeService.dispatch(setIsLoadingMap(true)) | ||
| this.notify(this.select()) | ||
| } else { | ||
| this.storeService.dispatch(removeLastBlacklistItem(), true) | ||
| this.dialogService.showErrorDialog("Excluding all buildings is not possible.", "Blacklist Error") | ||
| } |
There was a problem hiding this comment.
Is it possible to combine the logic in a way that it actually does remove all elements first, then you check if there are still elements and if there are, it's done, but if there are not, then you undo the last operation?
That would be more efficient :)
There was a problem hiding this comment.
If you do it that way, you'll first trigger that the blacklist has changed. Based on that event a lot of different services and components change their state, e.g. we calculate new metric data, since the maxValue might have changed. This would also trigger the re-decoration of the map.
We'd need to clone the previous map first in order to reset it, if all buildings will be excluded after decoration. We'd also need to notify all components and service, that they need to update their state again to the previous state.
There was a problem hiding this comment.
I personally would still do this, if it's reversible. It would be an optimization for the common case and a slow case for the case all folders are removed. This comment should not be considered blocking though.
There was a problem hiding this comment.
But this would require a clone. Cloning the map has the same complexity as checking if each building is excluded.
There was a problem hiding this comment.
Just for me to understand: how come we'd have to clone the map in that case? I would expect to actually change the map and then to reverse that action if we realize it runs into an issue (no files left).
There was a problem hiding this comment.
Well, cloning the map would be the easiest implementation. But now that I thought about this again:
While we update the map, we could add entries to a HashMap. Each entry represents the path of a node and it's previous state before we modified it.
If we run into a problem, we just traverse the tree again and check, if the path is in the HashMap and restore the previous state. We also remove the entry from the HashMap. Once the HashMap is empty, we're done.
I'll give this a shot
There was a problem hiding this comment.
I tried my best and pushed a solution, but I encounterd some issues that are quite hard to explain without code as a context.
Listening to blacklist-changes and as a reaction trigger a blacklist-change was tricky, since we block all other subscribers. e.g:
Blacklist all nodes -> store changes -> blacklist-changes -> trigger decoration -> dispatch blacklist-action -> store changes -> blacklist-changes -> Now the second blacklist-change is applied to all subscribers first and then we can execute the blacklist-change we triggered the first time.
This resulted in wrong data being displayed and wrong metrics being selected. I fixed this, by listening to the store directly and check for blacklist-events:
Blacklist all nodes -> store changes -> check if it was because of blacklist -> trigger decoration -> dispatch blacklist-action -> store changes -> blacklist-changes -> blacklist-changes from the first store-change.
…ding-last-building-error # Conflicts: # CHANGELOG.md # visualization/app/codeCharta/ui/nodeContextMenu/nodeContextMenu.component.spec.ts # visualization/app/codeCharta/ui/nodeContextMenu/nodeContextMenu.component.ts
…ding-last-building-error # Conflicts: # CHANGELOG.md # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.spec.ts # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.ts # visualization/app/codeCharta/ui/codeMap/codeMap.render.service.spec.ts # visualization/app/codeCharta/util/nodeDecorator.spec.ts # visualization/app/codeCharta/util/nodeDecorator.ts
visualization/app/codeCharta/state/store/fileSettings/blacklist/blacklist.service.spec.ts
Outdated
Show resolved
Hide resolved
| @@ -17,7 +17,7 @@ <h3 class="header-text">Flattened</h3> | |||
| </div> | |||
| </md-list-item> | |||
|
|
|||
| <md-list-item ng-repeat="item in $ctrl._viewModel.flatten | orderBy: ['path'] track by $index" title="{{::item.path}}"> | |||
| <md-list-item ng-repeat="item in $ctrl._viewModel.flatten | orderBy: ['path'] track by $index+item.path" title="{{::item.path}}"> | |||
There was a problem hiding this comment.
Is the path definitely required? If it's for testing: we could check the title to find the path :)
There was a problem hiding this comment.
When using track-by, we need to track by something that is actually unique. AngularJS usually applies a hash to each object used in ng-repeat, but with track-by we have to deal with this by ourselves. track by $index is used, when you have a list with duplicates. Usually, angularjs would throw an error since duplicates result in the same hash, but with $index the hash is now the index.
For some reason, if only the index is used here, we will have a blacklist in our view where each entry has the same path, even though they're different during runtime. They're just not displayed correctly. I don't know, why this is the case in this branch and not on main.
Edit: This also happens on main.
visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.spec.ts
Outdated
Show resolved
Hide resolved
…unused list of flattened nodes when checking for the number of excluded ones #901
…ding-last-building-error # Conflicts: # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.spec.ts # visualization/app/codeCharta/util/nodeDecorator.spec.ts # visualization/app/codeCharta/util/nodeDecorator.ts
Refactor change test data of codeMapPreRenderService to actually match the map
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
…raversing the tree #901
…ding-last-building-error # Conflicts: # visualization/app/codeCharta/state/store.service.ts # visualization/app/codeCharta/state/store/fileSettings/blacklist/blacklist.reducer.ts # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.ts
… object for silent dispatching #901
…ding-last-building-error # Conflicts: # visualization/app/codeCharta/ui/codeMap/codeMap.preRender.service.spec.ts # visualization/app/codeCharta/ui/codeMap/codeMap.render.service.spec.ts # visualization/app/codeCharta/util/nodeDecorator.spec.ts
|
Current state of this PR: |
Show error dialog when all buildings would be excluded
Please read the CONTRIBUTING.md before opening a PR.
fixes: #901
Description
This fix was a bit tricky. Before decorating the blacklist-attributes onto the renderMap, we have to check, if the number of excluded buildings would equal the number of buildings in general. If that's the case, show a dialog and revert the latest blacklist entry.
This has been achieved by refactoring the dataflow in the
codeMapPreRenderServiceand splitting the decoration process.Before: When we blacklist something, we recalculate the
metricDataand then trigger ametricDataAddedEvent.Now: When we blacklist something, we recalculate the
metricDataand then trigger ametricDataAddedEvent. However, we only decorate the map, if the map has not been decorated yet. We call thisdecorateNewMap. This only needs to be done once on a completly new map. For an already decorated map, we usedecorateExistingMap. This reduces unnecessary calculations and should improve the performance slightly.Screenshots or gifs