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.

Sign up for GitHub

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

Jump to bottom

Tech/move filestates to redux #853

Merged
merged 56 commits into from Mar 11, 2020
Merged

Tech/move filestates to redux #853

merged 56 commits into from Mar 11, 2020

Conversation

NearW
Copy link
Contributor

@NearW NearW commented Jan 31, 2020 •
edited

Move the filestates to the redux store

Description

This PR should prevent the fileStates from being mutated unpredictably by moving it to the redux store and only mutating it with redux actions. This should make the dataflow easier and also make future performance issues (cloning a lot) easier to solve.

  • Created a new folder model
  • Added class Files that handles all reducer functions and provides the functionality of the previous fileStateHelper class
  • Refactored some functions that passed a parameter through and through instead of just reading it from the store, where the parameter was needed

Notes to reviewer

  • When everything is reviewed and approved, we should move the codeCharta.model.ts into the model folder
  • Since this was a major refactoring, please take your time to give it a long manual 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
Add files to redux state
87a23dc
@NearW
Fixed wrong parenthesis in splitter template
695eef6
@NearW
Add model folder and move model to it
a3a8d69
@NearW
Add files that combine logic of fileStateService and fileStateHelper
b7497fa
@NearW
Add reducer implementation
f1938fa
@NearW
Add missing multiple mode test
ee302ca
@NearW
Merge remote-tracking branch 'remotes/origin/master' into tech/move-f…
5af56d5 
…ilestates-to-redux

# Conflicts:
#	visualization/app/codeCharta/state/injector.service.ts
#	visualization/app/codeCharta/state/state.module.ts
#	visualization/app/codeCharta/ui/codeMap/codeMap.arrow.service.spec.ts
#	visualization/app/codeCharta/ui/codeMap/codeMap.arrow.service.ts
#	visualization/app/codeCharta/ui/codeMap/threeViewer/threeSceneService.ts
#	visualization/app/codeCharta/ui/fileChooser/fileChooser.component.ts
@NearW
Merge remote-tracking branch 'remotes/origin/master' into tech/move-f…
c0fae6e 
…ilestates-to-redux
@NearW NearW added the tech For technical stories without user impact (=refactoring stories). label Jan 31, 2020
@alschmut
Copy link
Contributor

alschmut commented Jan 31, 2020 •
edited

@NearW are you sure to add the files (old: fileStates) to the fileSettings? Because in fact, a CCFile can contain fileSettings, which makes it recursive then. I would rather put them side by side with the fileSettings/dynamicSettings etc. on the top level.

@NearW
Copy link
Contributor Author

NearW commented Feb 6, 2020

@NearW are you sure to add the files (old: fileStates) to the fileSettings? Because in fact, a CCFile can contain fileSettings, which makes it recursive then. I would rather put them side by side with the fileSettings/dynamicSettings etc. on the top level.

I know. See the first todo checkbox. :D

@NearW
Refactor move files to rootReducer
c4a004b
@NearW
Merge remote-tracking branch 'remotes/origin/master' into tech/move-f…
f83a249 
…ilestates-to-redux

# Conflicts:
#	visualization/plopfile.js
@NearW
Add update files in redux store
1c764c0
@NearW
Refactor rename methods to indicate that parameter are multiple files
e9d9676
@NearW
Refactor clone files before setting them
052940b
@NearW
Refactor use lodash deepClone for now
36a4b65
@NearW
Refactor using CCAction instead of Action and make payload optional
cae3f4d
@NearW
Fix setSingle, setMultiple and setDelta not comparing correctly
3fefdfa
@NearW
Refactor replace read from fileStateService
3787f19
@NearW
Remove FileStateService and FileStateHelper
b73dba4
@NearW
Merge remote-tracking branch 'remotes/origin/master' into tech/move-f…
677b4a4 
…ilestates-to-redux

# Conflicts:
#	visualization/app/codeCharta/codeCharta.service.ts
#	visualization/app/codeCharta/state/metric.service.spec.ts
#	visualization/app/codeCharta/state/store.service.spec.ts
#	visualization/app/codeCharta/state/store.service.ts
#	visualization/app/codeCharta/state/store/appSettings/mapColors/mapColors.reducer.ts
#	visualization/app/codeCharta/state/store/dynamicSettings/colorRange/colorRange.reducer.ts
#	visualization/app/codeCharta/state/store/fileSettings/attributeTypes/attributeTypes.reducer.ts
#	visualization/app/codeCharta/state/store/fileSettings/attributeTypes/attributeTypes.service.ts
#	visualization/app/codeCharta/state/store/fileSettings/blacklist/blacklist.service.ts
#	visualization/app/codeCharta/state/store/fileSettings/edges/edges.service.ts
#	visualization/app/codeCharta/state/store/fileSettings/markedPackages/markedPackages.reducer.ts
#	visualization/app/codeCharta/state/store/fileSettings/markedPackages/markedPackages.service.ts
#	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/fileDownloader.ts
#	visualization/app/codeCharta/util/settingsMerger.spec.ts
#	visualization/app/codeCharta/util/settingsMerger.ts
@NearW
Fix split filesActions into newFilesImported and selectionChanged to …
ede9eaf 
…prevent autoFitting before map is built
@NearW
Refactor rename subscribe method
5455cb4
@NearW
Refactor use Files in viewModel
553f785
@NearW
Fix inverting files in multiple mode not updating the viewModel corre…
601642f 
…ctly
@NearW
Remove settingsMerger
2c11162
@NearW
Refactor replace lodash clone with rfdc
6048815
@NearW NearW changed the title WIP: Tech/move filestates to redux Tech/move filestates to redux Feb 14, 2020
NearW and others added 17 commits February 21, 2020 09:22
@NearW
Refactor flip if-Statement
9f02aa4
@NearW
Refactor use visibleFileStates
29eca76
@NearW
Refactor replace wrong method name in test descriptions
502b148
@NearW
Refactor replace test to expect unary metric exactly once
782ecbf
@NearW
Refactor flip if statement
38354be
@NearW @alschmut
Update visualization/app/codeCharta/state/store/files/files.reducer.s…
0e8d131 
…pec.ts

Co-Authored-By: Alexander Schmutz <aschmutz1@web.de>
@NearW @alschmut
Update visualization/app/codeCharta/state/store/fileSettings/markedPa…
8ecbe94 
…ckages/markedPackages.service.ts

Co-Authored-By: Alexander Schmutz <aschmutz1@web.de>
@NearW @alschmut
Update visualization/app/codeCharta/state/store/files/files.reducer.s…
c43889e 
…pec.ts

Co-Authored-By: Alexander Schmutz <aschmutz1@web.de>
@NearW @alschmut
Update visualization/app/codeCharta/state/store/files/files.reducer.s…
371d3da 
…pec.ts

Co-Authored-By: Alexander Schmutz <aschmutz1@web.de>
@NearW @alschmut
Update visualization/app/codeCharta/state/store/files/files.reducer.s…
336e132 
…pec.ts

Co-Authored-By: Alexander Schmutz <aschmutz1@web.de>
@NearW
Merge remote-tracking branch 'origin/tech/move-filestates-to-redux' i…
c2cd467 
…nto tech/move-filestates-to-redux
@NearW
Refactor wrap test in describe
3d562a4
@NearW
Update test description
0144738
@NearW
Refactor use fileStatesAvailable
143f1b8
@NearW
Fixed wrongly copy pasted test
62216b6
@NearW
Remove obsolete test
8cf76d3
@NearW
Refactor fileStatesAvailable usage
cfcdd53
@alschmut
Copy link
Contributor

alschmut commented Feb 27, 2020 •
edited

So I just run the visualisation for this branch, and found these two bugs:

undefined mode (fixed with commit)

  • Select Multiple mode
  • Open the mapSelection
  • Unselect all maps or click button None
    => now no mode is selected, the loadingMapGif is showing and
    => the mapSelection disappears, when clicking outside of the mapSelection menu
    Comment: While I kinda like that it's not setting the only available unary metric for all three metrics as it did before, we should find a solution here that behaves like before or better.

preselected multiple mode (not reproducable)

  • Select Multiple mode
  • Import two or more files
    => now sometimes the multiple mode is still selected, although it should by default select single mode after importing new files
  • EDIT: I can somehow not reproduce this bug anymore..

@alschmut alschmut self-assigned this Mar 1, 2020
@alschmut alschmut self-requested a review March 7, 2020 13:39
alschmut
alschmut approved these changes Mar 7, 2020
View reviewed changes
Copy link
Contributor

@alschmut alschmut left a comment

Choose a reason for hiding this comment

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

After implementing the requested changes and fixing the selection of zero files in Multiple mode, I am fine with merging this PR 🎉

@JaiJoPa
Copy link
Contributor

JaiJoPa commented Mar 11, 2020

After implementing the requested changes and fixing the selection of zero files in Multiple mode, I am fine with merging this PR 🎉

I like the fix! Thanks @alschmut

@JaiJoPa JaiJoPa merged commit f1be26d into master Mar 11, 2020
@JaiJoPa JaiJoPa deleted the tech/move-filestates-to-redux branch March 11, 2020 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alschmut alschmut alschmut approved these changes

@JaiJoPa JaiJoPa JaiJoPa approved these changes

Assignees

@alschmut alschmut

@JaiJoPa JaiJoPa

Labels
tech For technical stories without user impact (=refactoring stories).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@NearW @alschmut @BridgeAR @JaiJoPa