Skip to content

Consolidate Display nodes for re-categorization - #7759

Merged
User-Zhaoyang merged 2 commits into
DynamoDS:LibraryReorgfrom
User-Zhaoyang:LibraryReorg
Apr 7, 2017
Merged

Consolidate Display nodes for re-categorization#7759
User-Zhaoyang merged 2 commits into
DynamoDS:LibraryReorgfrom
User-Zhaoyang:LibraryReorg

Conversation

@User-Zhaoyang

@User-Zhaoyang User-Zhaoyang commented Apr 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Purpose

This pull request is to rename and recategorize the two nodes: ByGeometryColor and BySurfaceColor.

For this, the class name where the functions for these nodes stay is changed from "Display" to "GeometryColor". The namespace of the class is changed from "Display" to "Modifiers".

image

Because the class name has been changed, correspondingly the project name is also changed. Furthermore, migration from old nodes to new nodes is implemented through DSCoreNodes.Migrations.xml.

Two test cases have been added to ensure old nodes can be migrated to new nodes successfully.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@aparajit-pratap

FYIs

@benglin @riteshchandawar @monikaprabhu

@User-Zhaoyang User-Zhaoyang changed the title Consolidate Diplay nodes for re-categorization Consolidate Display nodes for re-categorization Apr 6, 2017
aparajit-pratap
aparajit-pratap reviewed Apr 6, 2017
View reviewed changes
Comment thread src/Libraries/CoreNodes/DSCoreNodes.Migrations.xml
</additionalAttributes>
</priorNameHint>
<priorNameHint>
<oldName>Display.Display.ByGeometryColor</oldName>

@aparajit-pratap aparajit-pratap Apr 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since this node is being renamed for the second time, will two migration entries for the same node work?

@aparajit-pratap

aparajit-pratap commented Apr 6, 2017

Copy link
Copy Markdown
Contributor

@Randy-Ma thanks for the changes and tests. I have just a few comments:

  • I don't think it is necessary to change the namespace from Display to Modifiers as that is an internal thing. The categorization is ultimately governed by the layoutspec.json file in librarie.js. I suppose it doesn't harm but if we keep the namespaces unchanged we can retain the old name of the dll as Display.dll. On the other hand it will help in terms of autocomplete in CBN as if there are classname conflicts, then the fully qualified name of the class will appear as Modifier.GeometryColor.ByXXX (which will mimic the node categorization) as opposed to Display.GeometryColor.ByXXX, which will be confusing.
  • I also see MeshDisplay.ByMeshColor has been renamed to GeometryColor.ByMeshColor as per the spreadsheet or is this part of another JIRA task?
  • I guess we can also get rid of the *_Cusomization.xml files as they were needed only for the older WPF UI?

@mjkkirschner

mjkkirschner commented Apr 6, 2017

Copy link
Copy Markdown
Member

@ramramps @jnealb

@ramramps

ramramps commented Apr 6, 2017

Copy link
Copy Markdown
Collaborator

Does this require changes in Reach / Flood?

@mjkkirschner

mjkkirschner commented Apr 6, 2017

Copy link
Copy Markdown
Member

@ramramps to support old graphs I believe it will, for new ones I dont think so.

@ramramps

ramramps commented Apr 6, 2017

Copy link
Copy Markdown
Collaborator

but if the migration is in place, the old graphs should work as expected. or is it because changing the name in Flood will cause the old graph to break?

@ramramps

ramramps commented Apr 6, 2017

Copy link
Copy Markdown
Collaborator

@mjkkirschner as discussed, let's create a task for Reach to fetch the migration strategy for a specific node. @Tanga I am thinking it should fall under the epic - Library Recategorization.

@QilongTang

QilongTang commented Apr 6, 2017

Copy link
Copy Markdown
Contributor

https://jira.autodesk.com/browse/DYN-740

@User-Zhaoyang

User-Zhaoyang commented Apr 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@aparajit-pratap
Thanks for the comments! As for each of them correspondingly:

  • I prefer to change "Display" to "Modifiers". Otherwise it will appear to be inconsistent to users like you have mentioned unless we can make these customizable in the code block node.
  • You are right, here I have only addressed two nodes, the other nodes can not be addressed here.
  • Regarding to the customization files, I think we can address them separately if required because there are quite a few such files.

@User-Zhaoyang

User-Zhaoyang commented Apr 7, 2017

Copy link
Copy Markdown
Contributor Author

@aparajit-pratap
I have added one more test case for migrating the very old ByGeometryColor node from 0.8.1.

@aparajit-pratap

aparajit-pratap commented Apr 7, 2017

Copy link
Copy Markdown
Contributor

Right MeshDisplay.ByMeshColor is actually implemented in MeshToolkit. We can assign a separate task for that in the next sprint @riteshchandawar. @Randy-Ma LGTM for now.

@User-Zhaoyang
User-Zhaoyang merged commit 2c7c5a3 into DynamoDS:LibraryReorg Apr 7, 2017
aparajit-pratap
aparajit-pratap reviewed Apr 18, 2017
View reviewed changes
Comment thread src/Libraries/CoreNodes/DSCoreNodes.Migrations.xml
<priorNameHint>
<oldName>DSCore.Display.ByGeometryColor</oldName>
<newName>Display.Display.ByGeometryColor</newName>
<newName>Modifiers.GeometryColor.ByGeometryColor</newName>

@aparajit-pratap aparajit-pratap Apr 18, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Randy-Ma I would reconsider renaming namespaces and suggest changing only class names. We need to keep in mind that renaming namespaces also affects namespace mapping in Thunderstorm @gregmarr. Please refer to @ikeough's comments on the #dynamo-design-script slack channel: https://autodesk.slack.com/archives/C187950LA/p1491917642623367

This was referenced Apr 24, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aparajit-pratap aparajit-pratap aparajit-pratap left review comments

Assignees

@User-Zhaoyang User-Zhaoyang

@aparajit-pratap aparajit-pratap

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@User-Zhaoyang @aparajit-pratap @mjkkirschner @ramramps @QilongTang