Improve the scripting environment

[jkarder]

Some improvements can be made:

• IEnumerable<T> extension methods should be usable with the variables collection.
• If an INamedItem is stored in the variables collection, the name of the item can automatically become the name of the variable.
• Renaming variables should be possible by pressing F2.
• Removing variables should be faster.
• Selecting multiple variables in the VariableStoreView should be possible.
• Scrolling inside the code editor should be possible when executing scripts.

This ticket has to be merged into stable before #2077

[jkarder]

r11436:

• IEnumerable<T> extension methods are supported by the variables collection
• INamedItem variables get the item's name per default
• variables can be renamed by pressing F2
• variables are removed faster
• multiple variables can be selected
• the code editor supports scrolling while scripts are executed

[jkarder]

r11440: fixed UpdateVariable

[mkommend]

Reviewed r11436 and r11440.

Functionality

• Will be done in #2077 While the script is running there is no indication that the code cannot be changed (grey background) and the cursor should be hidden.
• When multiple items are selected, renaming should not be possible.
• When cloning a variable from within the VariableStore the name of the variable is not taken into account for the creating the name of the new variable. E.g., Add a new GA to the script and rename the variable to any value. After cloning the name of the cloned variable is "Genetic Algorithm".

CSharpScriptBase

• This would break existing scripts. Variables and Vars field in CSharpScriptBase should be implemented as a property with a protected getter and private setter.

VariableStoreView

• I would only allow IDeepCloneables to be dropped on the VariableStoreView.
• AddVariable (line 141, 142): Why does the null check differ in these two lines?
• UpdateVariable and AddVariable contain lots of similar code (ListViewItem configuration) that should be extracted in a separate method.
• variableListView_DragDrop (line 251): I would invert the condition (e.Effect == DDE.Copy) to (e.Effect != DDE.Copy) and return immediately if it is fulfilled. This reduces the nesting level of the method and makes it more understandable.
• variableListView_DragDrop (line 255): Avoid manual creation of a cloner and use IDeepCloneable.Clone() instead.
• variableListView_DragDrop (line 255): nameValid is a poorly chosen variable name, because it just checks if the name is not null

I noticed that you rarely use empty lines for code organization. IMHO, carefully placed empty lines could improve the readability of the code drastically.

[jkarder]

r11471: allow invalid c# identifiers after label edits

[jkarder]

r11474: applied some of the changes suggested by mkommend in comment:10:ticket:2262

• renaming is now only possible if one variable is selected; the variable does not have to be focused
• avoided manual creation of cloner
• changed drag-and-drop logic; existing variable names are now taken into account when cloning INamedItems

[mkommend]

r11477: Refactored Script.GetAssemblies to get rid of the try/catch block.

[jkarder]

r11479: worked on variable management of VariableStoreView

[jkarder]

r11480:

• worked on variable management of VariableStoreView
• fixed ReadOnly and Locked state changes in ScriptView
• minor code improvements

[swagner]

As I need similar functionality for optimization networks, I had a look at the scripting code and have some comments:

• in Script.Compile a more specific exception than Exception should be thrown (e.g. InvalidOperationException)
• Script.DoCompile should call CodeProvider.CompileAssemblyFromSource instead of CodeProvider.CompileAssemblyFromDom
• the call of CodeProvider.GenerateCodeFromCompileUnit is not necessary in Script.DoCompile
• it would be nice to read the CodeTemplate code from an embedded resource instead of using a large string literal in CSharpScript (I have done it that way for programmable network items and programmable nodes in optimization networks)
• the code field should be initialized with CodeTemplate instead of string.Empty in the Script ctor
• Used in the CodeEditor 3.4 is there a special reason why the TextEditorTextChanged event is used in ScriptView to update Content.Code instead of the Validated event of the codeEditor? (in the ProgrammableOperatorView the Validated event is used)
• in ScriptView codeEditor.ShowCompileErrors should be used to visualize errors in the code
• ScriptView.ContentOnCodeChanged doesn't use Invoke
• calling OnContentChanged in ScriptView.Compile is not optimal, as the assignment codeEditor.UserCode = Content.Code immediately removes all code markups again when visualizing errors using codeEditor.ShowCompileErrors; furthermore I am not sure, if repeatedly calling codeEditor.AddAssembly doesn't have some side effects
• Will be addressed in #2077 the error markups of codeEditor.ShowCompileErrors can be shown again when double clicking on the error list
• it would be nice to set the cursor in the code editor automatically on the error position when double clicking on an error in the error list (but this is currently not supported by the code editor and therefore would require some additional changes in the code editor)

[swagner]

One more comment:

• CSharpScript.Execute should be renamed into CSharpScript.ExecuteAsync (cf. #2258)

[jkarder]

r11605: VariableStore now implements IItem

[jkarder]

r11657: applied some of the changes suggested by swagner in comment:17:ticket:2262

• added highlighting of current line
• added error markers and bookmarks
• fixed <Ctrl> + <Backspace> bug
• minor code changes

[bburlacu]

Improvement idea: it would be cool if the script would display an Execution Time like the other algorithms.

[ascheibe]

So the remaining tasks for this ticket are (the rest will be done in #2077):

• I would only allow IDeepCloneables to be dropped on the VariableStoreView.
• it would be nice to read the CodeTemplate code from an embedded resource instead of using a large string literal in CSharpScript (I have done it that way for programmable network items and programmable nodes in optimization networks)
• CSharpScript.Execute should be renamed into CSharpScript.ExecuteAsync (cf. #2258)
• Improvement idea: it would be cool if the script would display an Execution Time like the other algorithms.

[jkarder]

r11721:

• fixed SetEnabledStateOfControls in VariableStoreView
• the code template for a CSharpScript is now read from an embedded resource

[jkarder]

r11734:

• only IDeepCloneables can be dropped on the VariableStoreView
• renamed CSharpScript.Execute to CSharpScript.ExecuteAsync

[jkarder]

r11735: updated ScriptingUtils

[jkarder]

r11787: refactored code template loading

[jkarder]

r11788: added custom tool namespace

[jkarder]

r11789: refactored scripting unit tests

• changed code template loading
• minor code changes

[ascheibe]

Reviewed r11436, r11440, r11471, r11474, r11477, r11479, r11480, r11605, r11657, r11721, r11734, r11735, r11787, r11787, r11789. Seems ok, just 2 things:

• VariableStoreView.cs: Please move SafeVariableNameReges/DefaultVariableName to the top of the class.
• As a side note for the future: I don't really understand the seperation between Script and CSharpScript. Script compiles C# code but can't really run it. CSharpScript runs C# scripts and has a variable store. I think that this should be refactored if/when we add another language. I would make Script abstract and language independent and put all language-specific stuff into classes that inherit from Script.

[jkarder]

r11826: moved SafeVariableNameRegex and DefaultVariableName to the top of the class

[abeham]

While working on the programmable problem I noticed an issue with exceptions coming from Compile(). It can happen that a script compiles perfectly, but still that the resulting object may not work. E.g. I compile a problem definition that doesn't specify an encoding. Compilation is fine, but when assigning the instance of that definition to the programmable problem I'll get an exception. Currently, the script view doesn't react to these errors. The problem is here:

      try {
        Content.Compile();
        outputTextBox.AppendText(CompilationSucceededMessage);
        UpdateInfoTextLabel(CompilationSucceededMessage, Color.DarkGreen);
        return true;
      } catch (InvalidOperationException) {
        if (Content.CompileErrors.HasErrors) {
          outputTextBox.AppendText(CompilationFailedMessage);
          UpdateInfoTextLabel(CompilationFailedMessage, Color.DarkRed);
          return false;
        } else {
          outputTextBox.AppendText(CompilationSucceededMessage);
          UpdateInfoTextLabel(CompilationSucceededMessage, Color.DarkGreen);
          return true;
        }
      }

In the else branch the compilation is shown successful, even though there are errors. I think we should instead use the error dialog to show this error to the user. Otherwise the InvalidOperationException is just ignored. Maybe there is a reason why the InvalidOperationException can be ignored if there are no compile errors, but it is not apparent.

[jkarder]

I will address this in #2303.

Currently an InvalidOperationException will be thrown if errors or warnings were found during compilation. The CompilerErrorCollection.HasErrors only indicates whether the collection contains errors. The error collection can also contain warnings and if only warnings are present, the compilation was successful.

Replying to abeham:

While working on the programmable problem I noticed an issue with exceptions coming from Compile(). It can happen that a script compiles perfectly, but still that the resulting object may not work. E.g. I compile a problem definition that doesn't specify an encoding. Compilation is fine, but when assigning the instance of that definition to the programmable problem I'll get an exception. Currently, the script view doesn't react to these errors. The problem is here:

      try {
        Content.Compile();
        outputTextBox.AppendText(CompilationSucceededMessage);
        UpdateInfoTextLabel(CompilationSucceededMessage, Color.DarkGreen);
        return true;
      } catch (InvalidOperationException) {
        if (Content.CompileErrors.HasErrors) {
          outputTextBox.AppendText(CompilationFailedMessage);
          UpdateInfoTextLabel(CompilationFailedMessage, Color.DarkRed);
          return false;
        } else {
          outputTextBox.AppendText(CompilationSucceededMessage);
          UpdateInfoTextLabel(CompilationSucceededMessage, Color.DarkGreen);
          return true;
        }
      }

In the else branch the compilation is shown successful, even though there are errors. I think we should instead use the error dialog to show this error to the user. Otherwise the InvalidOperationException is just ignored. Maybe there is a reason why the InvalidOperationException can be ignored if there are no compile errors, but it is not apparent.

[jkarder]

r11907: merged r11436, r11440, r11471, r11474, r11477, r11479, r11480, r11605, r11657, r11721, r11734, r11735, r11787, r11788, r11789 and r11826 into stable