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

Python template support #8034

Closed
wants to merge 15 commits into from
Closed

Conversation

radumg
Copy link
Collaborator

@radumg radumg commented Jul 12, 2017

Purpose

This PR fixes #7604 , wish for adding a Python template that can be used to populate any Python Script nodes when adding to workspace.

Current behaviour

  • user adds Python Script node to canvas
  • the node is populated with a few lines of code, but these are hard-coded in the Dynamo source code, so users are unable to change them

New behaviour

  • user adds Python Script node to canvas
  • Dynamo checks if the PythonTemplate.py file exists at the user location root (%appdata%/Dynamo/Core/{version}/) and if it does, it reads the file and populates the Python script node with its contents
  • if the file doesn't exits or it's empty, Dynamo falls back on the hard-coded behaviour

pythontemplate

This allows users and organisations to rollout the template as part of users profiles or as logon scripts.

Implementation

  • refactored PreferenceSettings class to include code regions, moving properties into their logical group, also fixing some comments/documentation at same time, making it clearer.
  • implemented a PythonTemplateFilePath property in the settings class so it serialises and deserialises with user settings
  • implemented required properties in the interfaces IPreferences, IPathResolver and PathManager class
  • added a few additional lines to the hardcoded python template, providing pointers to users where to start writing code and explaining the first lines. this is following my observations in-person and on forums that most new users struggle with those 2 things : where to put code and what those bits mean.
  • replaced \n with .NET standard Environment.NewLine line breaks, should behave more predictably when compiled on different platforms

Limitations

  • the name and location of template file is hardcoded. however, this is consistent/same as DynamoSettings.XML file itself. Since the interfaces implement the property, it can be exposed via UI in later PR
    path
  • I couldn't figure out how to access the current DynamoModel instance from the PythonNodeModel class to retrieve current settings, so the template is read every time a Python Script node is added to canvas. Would welcome some pointers here as both the Dynamo API documentation and the class annotations are lackluster. Would've expected either a static class or some public methods that expose this.

Declarations

  • 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

@kronz
@mjkkirschner
@QilongTang

FYIs

@ksobon , @ThomasMahon, @wynged , @eibre , @andydandy74

radumg added 5 commits July 11, 2017 23:08
- introduced code regions
- moved class properties to appropriate regions
- fixed a few summary annotations to make it clearer what they do/return and adhere to Microsoft standards a bit better
- added public property to class for PythonTemplateFilePath
- added default value in the Settings constructor
- builds fine
- creates the correct python template file path
- serialises it to XML file fine
Implemented support for the file path, following the model of the PreferencesFilePath.
- when adding a Python node, the template is read
- added conditional load & fallback on hardcoded text if template is not found
- replaced "\n" with `Environment.NewLine` in hardcoded template
- added new comment lines to default template, using Resources to make it easier for beginners
- fixed typo in existing Resources & XAML
@brencass
Copy link

brencass commented Jul 12, 2017

Sterling work and this is a great addition.

I have a small comment; Instead of the location being hard coded as being in a user location, could this location not be set from within the Dynamosettings.xml file. If not filled in or used it will go back to default location?

This will allow companies to have a central location for company standards that includes this python template and dynamo template.

@radumg
Copy link
Collaborator Author

radumg commented Jul 12, 2017

Thanks @brencass !

Really good suggestion - that's what i had in mind too and this PR does indeed store and read the location in the DynamoSettings.XML file, so that bit is already working.
What I couldn't do is access that setting (without building path, reading & parsing XML again) from within the code that initialises the Python Script node - it was already 3am by that point though, so i might have been a bit blind 😁

If someone a bit more familiar with the codebase ( @ke-yu, @QilongTang , @mjkkirschner ) can weight in and point me in the right direction, i can connect the 2 dots as originally intended.

@radumg radumg changed the base branch from RC1.3.0_master to RC1.3.1_master July 12, 2017 13:56
radumg and others added 10 commits July 12, 2017 14:57
it also propagates changes made to settings in Dynamo
- renamed static prop to PublicPreferenceSettings, essentially adding `Public` in front of existing field
- added lines to update the static prop where the `PreferenceSettings` gets updated in the model
- set static prop to be public GET, private SET
- tested with build, changes are picked up by Python nodes just fine
added public Static preferences
- added an extra spacing line
- changed back the default output so newly added Python node does not throw an exception due to missing variable `yourOutputVariableHere`
added spaces after `#` Python comment marker
@radumg
Copy link
Collaborator Author

radumg commented Jul 17, 2017

Ok, so I implemented requested change by @brencass above and aligned it to what I originally envisioned.

New behaviour

  • user adds Python Script node to canvas
  • Dynamo checks the global settings for a specified PythonTemplateFilePath setting

dynamo settings file

  • if that property is not found, it gets initialised with default value of : user root location + PythonTemplate.py.
  • if the file exists at the indicated path (custom one or default), it reads the file and populates the Python script node with its contents

Python template file in external editor :
python template
Python template read into PythonNode when node is added to canvas
python script in dynamo

  • if the file doesn't exist or it's empty, Dynamo falls back on the hard-coded behaviour

python invalid

Implementation

Added a public static property called PublicPreferenceSettings to DynamoModel class, so that nodes and other components that are not fully aware of the context they're executed in can still access these properties.

The property should be thread-safe as it's read-only :

  • read : public access
  • write : private access

Also added behaviour to update the static property whenever the instance-based PreferenceSettings gets updated within the DynamoModel class.

Tested by

  • building entire solution
  • running DynamoSandbox
  • changing Dynamo settings (such as show connectors or adding a new custom package path)
  • nodes reading the static property see these updates

Notes :

  • you can't (from what I can tell and what @mjkkirschner hinted at) get the current DynamoModel context from a NodeModel such as the Python Node
  • this property duplicates the contents of the PreferenceSettings property in current instance of DynamoModel, so any time this prop is updated, the static one is also updated
  • this duplication is rather inelegant but I don't see another way (rebuilding the settings file path and reading + parsing XML file for each Python node added to canvas is worse I think as it introduces duplicate code)

@radumg
Copy link
Collaborator Author

radumg commented Jul 17, 2017

@Racel , @ikeough - could you please have a look and let me know if this is something you'd want merged in and if any further changes are required ?

@Racel
Copy link
Contributor

Racel commented Jul 18, 2017

@radumg - We have a meeting to review this today. Will discuss with the team and get back to you.

@Racel
Copy link
Contributor

Racel commented Jul 18, 2017

@radumg - After review with the team, we want to consider other approaches than changing DynamoModel. @mjkkirschner will follow-up with some suggestions to think about.

Additionally, we noticed that your PRs were made against the 1.3.1 Release Candidate. In order for your changes to appear in the next stable release of Dynamo, please make a PR against master.

@radumg
Copy link
Collaborator Author

radumg commented Jul 18, 2017

Thank you @Racel for the feedback and quick reply 🎉 !
Looking forward to hearing @mjkkirschner 's suggestions - I agree changing the DynamoModel isn't ideal so happy to make changes - especially since i'm using this as rehearsal for adding DYN template support.

As for the PR branches, my bad : I assumed 1.3.1 would be the next stable release since there wasn't anything published after 1.3. It's since become apparent it was a Revit 2018.1-only release so I'll rebase on master branch and deal with any conflicts.

@radumg
Copy link
Collaborator Author

radumg commented Jul 18, 2017

@mjkkirschner , in anticipation of your suggestions : my initial instinct was to turn the PreferenceSettings class to static and refactor references to it as needed.
This however introduces the question of how this will be handled when support for multiple workspaces is added in 2.0 : my view is since they're global settings, they don't need to be in the DynamoModel in the first place, as they should not vary across models/workspaces.

This is obviously a significant departure from current approach & also a UX question, so i await instructions on what you think will be best.
Also, this has been a really good learning opportunity for me, thanks to the team for taking the time to review 👍

@mjkkirschner
Copy link
Member

@radumg so here are some alternatives I have not given much thought 😉 .

  • from python node raise an event requesting the path, dynamoModel can handle this and send back the path - it could be a general mechanism for requesting any preference setting even. This might be added to the DynamoServices project.

  • a static property on the PreferencesSettings class instead of DynamoModel.

  • a static property added to Dynamo services.

  • Python Node has a customization so it would be possible to find the dynamoModel through the DynamoViewModel, but I would advise against that, it means the execution would be tied to the view layer.

In general, I think adding more properties to DynamoModel is not great as it's already a giant class, so I'd rather if we have to that we add it somewhere specific.

let us know what you think of those.

@radumg
Copy link
Collaborator Author

radumg commented Jul 30, 2017

hi @mjkkirschner , thanks for all the options above! And sorry for the delay in replying, we've been busy organising the latest UK Dynamo User Group meeting last week.

Options feedback

In terms of the options, my notes on them would be :

from python node raise an event
This could be a good option to generally fetch properties from within nodes,
the drawbacks I see would be :

potential performance issues in waiting for the event callback when adding a new node ?
(I don't know what order the event requests are processed in so am wondering
if it's possible the UI layer would be locked waiting for this callback)
introduces additional code in each node/codebase requiring access to these properties
I might have a follow-up question or 2 about this one :D

a static property on the PreferencesSettings class instead of DynamoModel

quickest one to implement - happy to go ahead with this one.

a static property added to Dynamo services
the only place I can find that's vaguely suitable to place the static property would be the `IExecutionSession` interface - suggesting to me that `DynamoServices` might not be the right or expected place to implement this after all, since it would still require adding the property to the `DynamoModel` class which implements that interface.

Python Node customization
agree

Suggested options

That would leave us I think with 3 main options :

  1. a static property on the PreferencesSettings class
  2. a static PreferencesSettings class
  3. event handling on DynamoServices project

I'd gravitate towards option 2 above as a more long-term solution, I'm happy to go through implications and resolve. The lens I see this through is that once this PR is resolved, i'll be working on DYN template support next.

Still requires the answer to my question above When support for multiple workspaces is added, does each DynamoModel still require their own Preferences ?
If the answer is No, then

  • the PreferenceSettings class can be static
  • could also simplify the DynamoModel class since all the preference properties are moved to static PreferenceSettings class

Let me know which option you want me to go for,
1 - static property in PreferenceSettings class
2 - static PreferenceSettings class
3 - event handling in DynamoServices

Cheers, Radu

@mjkkirschner
Copy link
Member

mjkkirschner commented Aug 17, 2017

@radumg sorry for the delay here.
I think there might be someway to adding this info todynamoServicesbut as I looked through it not with any of the constructs we currently have there as they are all event based... let's do option number 1 for now and add a static property specifically for thepythonTemplatePath to the preferenceSettings class. This will let us control what preferences are made accessible to other nodes deliberately. Sound good?

@mjkkirschner
Copy link
Member

@radumg to clarify:
nubmer 1 in your final list :).
1 - static property in PreferenceSettings class

/// Full path to the python template py file. This setting file is specific
/// to the current user.
/// </summary>
string PythonTemplateFilePath { get; }
Copy link
Member

Choose a reason for hiding this comment

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

because you are changing the interface here you may need to update: https://github.com/DynamoDS/DynamoRevit/blob/Revit2017/src/DynamoRevit/RevitPathResolver.cs

in a related PR to avoid breaking the Revit build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @mjkkirschner , was definitely not aware of Dynamo Revit was implementing its own class from this interface. Will try and keep an eye out for that in the future.

@@ -304,6 +322,8 @@ public PreferenceSettings()
ShowDetailedLayout = true;
NamespacesToExcludeFromLibrary = new List<string>();

PythonTemplateFilePath = new PathManager(new PathManagerParams()).PythonTemplateFilePath;
Copy link
Member

Choose a reason for hiding this comment

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

I've not investigated far enough to know, but do you actually need to create a new path manager just to pull this property out, don't we create a pathmanager somewhere else during dynamoModel initialization?

Copy link
Collaborator Author

@radumg radumg Aug 17, 2017

Choose a reason for hiding this comment

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

Will investigate further but from what I remember, there wasn't a mechanism to get to the DynamoModel.

@@ -45,7 +45,7 @@
// to distinguish one build from another. AssemblyFileVersion is specified
// in AssemblyVersionInfo.cs so that it can be easily incremented by the
// automated build process.
[assembly: AssemblyVersion("1.3.1.1514")]
[assembly: AssemblyVersion("1.3.1.1977")]
Copy link
Member

Choose a reason for hiding this comment

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

I think you should send future PRs to master - and no need to commit this file, its auto generated.

Copy link
Collaborator Author

@radumg radumg Aug 17, 2017

Choose a reason for hiding this comment

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

Will do 👍 I originally based this one off the 1.3.1 branch thinking that would be the next release (it wasn't out yet). Maybe it would be a good idea to add to the wiki a bit about git branch management and how the team uses them, which one is current/release/etc ? I remember first PR I did was highly confusing how it was cherry-picked when merged etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dug around...can't figure out why this file was included, it's in the .gitignore for the repo, which is what my cloned version uses too.
Thanks for catching it, I'll exclude from future PRs.

@radumg
Copy link
Collaborator Author

radumg commented Aug 17, 2017

Thanks @mjkkirschner for the detailed feedback & review, will get working on that option asap 👍
I'll open a new PR based on master for it as I tried re-basing this one and it's too messy.

FYI, I had a working version of the static PreferenceSettings property on the DynamoModel too, in case you were wondering what the consequences of that option would've been.

Quick question : when is the team aiming to finish round of PR merges for next release ? I would like to get this one (should be ready this weekend) in there but also the next one which will look at support for DYN templates too.

radumg added a commit to radumg/Dynamo that referenced this pull request Aug 21, 2017
gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review)
@radumg radumg mentioned this pull request Aug 21, 2017
8 tasks
@radumg
Copy link
Collaborator Author

radumg commented Aug 21, 2017

created a new PR #8122 based on master branch, please see that for the implementation of what was discussed above.

@radumg radumg closed this Aug 21, 2017
radumg added a commit to radumg/Dynamo that referenced this pull request Aug 21, 2017
actually gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review)
forgot the PathManager in last commit.
jiafeng5513 pushed a commit to jiafeng5513/Expressior that referenced this pull request Oct 6, 2018
gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS/Dynamo#8034 (review)
jiafeng5513 pushed a commit to jiafeng5513/Expressior that referenced this pull request Oct 6, 2018
actually gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS/Dynamo#8034 (review)
forgot the PathManager in last commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants