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

Improve Python script editing experience #8182

Merged
merged 7 commits into from
Oct 16, 2017

Conversation

Dewb
Copy link
Contributor

@Dewb Dewb commented Sep 15, 2017

Purpose

This PR makes some incremental improvements to the Python script editor windows to address some common complaints and make writing, testing, and iterating complex Python scripts a little bit easier.

These changes are primarily to the WPF UI around Python. It does not change anything about how Python scripts are executed, but it does give the user more control over when Python nodes are reexecuted.

  • Allows the Python editor window to stay open
  • Allows multiple Python editor windows to be open at once
  • Makes the title of a Python editor window match the Python node's title
  • Adds a Run button to the Python editor to update the code, mark the Python node changed and re-execute the downstream parts of the graph

This addresses the following issues in whole or in part:
#5137 Python script editor window titles should match node titles
#5076 Add a Run button to the Python script editor
#5068 Allow multiple Python script editors to be open at once
#3378 Python improvements

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.

pythonux

Reviewers

FYIs

@wynged
Copy link
Contributor

wynged commented Sep 20, 2017 via email

@epeter-
Copy link

epeter- commented Sep 25, 2017

perfect!!!!!!!!!!!!!!!!!!!

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

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

@Dewb thanks for these changes! I have some minor comments on name changes and one question.

<value>Accept Changes</value>
<value>Save Changes</value>
</data>
<data name="PythonScriptEditorTestButton" xml:space="preserve">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simply name this PythonScriptEditorRunButton as it is essentially the Run button for the Python script editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I changed the names of the buttons and decided not to change the underlying methods to make it clearer that it's a cosmetic change, but in hindsight changing the method names is definitely better for long-term maintenance.

@@ -117,22 +122,37 @@ private void OnTextAreaTextEntered(object sender, TextCompositionEventArgs e)

private void OnAcceptClicked(object sender, RoutedEventArgs e)
{
DialogResult = true;
UpdateScript(editText.Text);
this.Close();
}

private void OnCancelClicked(object sender, RoutedEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to OnRevertClicked just so that the new intent is clearer or was Cancel doing the same thing before?

nodeModel.OnNodeModified();
}

private void OnTestClicked(object sender, RoutedEventArgs e)
Copy link
Contributor

Choose a reason for hiding this comment

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

OnRunClicked?

private void OnTestClicked(object sender, RoutedEventArgs e)
{
UpdateScript(editText.Text);
if (dynamoViewModel.HomeSpace.RunSettings.RunType != RunType.Automatic)
Copy link
Contributor

@aparajit-pratap aparajit-pratap Sep 28, 2017

Choose a reason for hiding this comment

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

So the Run button in the editor will have no effect unless the run mode in Dynamo is set to Automatic? Not sure if some users may expect this Run button to override the Dynamo mode setting and still execute the graph in manual Run mode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we would expect it to do that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run always triggers a re-execution of the changes to the Python node. In automatic mode, it merely updates the script and marks the node as modified; automatic mode takes care of the rest. In manual or periodic mode, an execution is explicitly triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sorry for some reason I read that the other way round; missed the !=. Thanks.

@radumg
Copy link
Collaborator

radumg commented Oct 11, 2017

perhaps this might be better suited in a different PR or separate unit of work, but i'd like to make a suggestion since you're tackling the Python WPF side of things :

it would be awesome to show output from the Python console somewhere in the Dynamo UI.

People have been asking for better de-bugging for a very long time and think this would satisfy at least some of the requests :
#7974
https://github.com/DynamoDS/Dynamo/issues/6512
https://github.com/DynamoDS/Dynamo/issues/4309

As for implementation, the output of Python console could either be routed to the Dynamo console or I imagine a preview-like bubble underneath the editing window would work too (or within it, don't mind) - it would also go nicely together with that new run button.

Not long ago, I had a hacky prototype of Python console output being re-routed to a text file - so I know there's support in IronPython for this and it's doable.

@Dewb
Copy link
Contributor Author

Dewb commented Oct 15, 2017

@radumg For sure. I went over the list of Python pain points and these seemed like the easiest/highest value. I hope to have some time to tackle the slightly trickier ones soon, and sending output to the Dynamo console is high on that list.

@Dewb
Copy link
Contributor Author

Dewb commented Oct 15, 2017

@aparajit-pratap The last commit should address your review comments, apologies for not pinging you earlier.

@mostaphaRoudsari
Copy link

🎉 🎉 🎉 🎈

@aparajit-pratap
Copy link
Contributor

Thanks @Dewb! Merging ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants