Opened 2 years ago

Closed 2 years ago

#2262 closed enhancement (done)

Improve the scripting environment

Reported by: jkarder Owned by: jkarder
Priority: medium Milestone: HeuristicLab 3.3.11
Component: Scripting Version: 3.3.10
Keywords: Cc:

Description (last modified by ascheibe)

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

Change History (39)

comment:1 Changed 2 years ago by jkarder

  • Status changed from new to accepted

comment:2 Changed 2 years ago by jkarder

  • Description modified (diff)

comment:3 Changed 2 years ago by jkarder

  • Description modified (diff)

comment:4 follow-up: Changed 2 years ago by 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

comment:5 Changed 2 years ago by jkarder

  • Owner changed from jkarder to mkommend
  • Status changed from accepted to reviewing

comment:6 Changed 2 years ago by jkarder

  • Owner changed from mkommend to jkarder
  • Status changed from reviewing to assigned

comment:7 Changed 2 years ago by jkarder

  • Status changed from assigned to accepted

comment:8 Changed 2 years ago by jkarder

r11440: fixed UpdateVariable

comment:9 Changed 2 years ago by jkarder

  • Owner changed from jkarder to mkommend
  • Status changed from accepted to reviewing

comment:10 in reply to: ↑ 4 Changed 2 years ago by mkommend

  • Owner changed from mkommend to jkarder
  • Status changed from reviewing to assigned

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.

Last edited 2 years ago by jkarder (previous) (diff)

comment:11 Changed 2 years ago by jkarder

  • Status changed from assigned to accepted

comment:12 Changed 2 years ago by jkarder

r11471: allow invalid c# identifiers after label edits

comment:13 Changed 2 years ago by 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

comment:14 Changed 2 years ago by mkommend

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

comment:15 Changed 2 years ago by jkarder

r11479: worked on variable management of VariableStoreView

comment:16 Changed 2 years ago by jkarder

r11480:

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

comment:17 Changed 2 years ago by 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)
Last edited 2 years ago by jkarder (previous) (diff)

comment:18 Changed 2 years ago by swagner

One more comment:

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

comment:19 Changed 2 years ago by jkarder

r11605: VariableStore now implements IItem

comment:20 Changed 2 years ago by 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

comment:21 Changed 2 years ago by bburlacu

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

comment:22 Changed 2 years ago by ascheibe

  • Description modified (diff)

comment:23 Changed 2 years ago by ascheibe

  • Description modified (diff)

comment:24 Changed 2 years ago by 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)
  • Will be addressed in #2298 Improvement idea: it would be cool if the script would display an Execution Time like the other algorithms.
Last edited 2 years ago by jkarder (previous) (diff)

comment:25 Changed 2 years ago by jkarder

r11721:

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

comment:26 Changed 2 years ago by jkarder

r11734:

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

comment:27 Changed 2 years ago by jkarder

r11735: updated ScriptingUtils

comment:28 Changed 2 years ago by jkarder

r11787: refactored code template loading

comment:29 Changed 2 years ago by jkarder

r11788: added custom tool namespace

comment:30 Changed 2 years ago by jkarder

r11789: refactored scripting unit tests

  • changed code template loading
  • minor code changes

comment:31 Changed 2 years ago by jkarder

  • Owner changed from jkarder to ascheibe
  • Status changed from accepted to reviewing

comment:32 Changed 2 years ago by ascheibe

  • Owner changed from ascheibe to jkarder
  • Status changed from reviewing to readytorelease

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.
Last edited 2 years ago by jkarder (previous) (diff)

comment:33 Changed 2 years ago by jkarder

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

comment:34 follow-up: Changed 2 years ago by 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.

comment:35 Changed 2 years ago by jkarder

  • Status changed from readytorelease to assigned

comment:36 Changed 2 years ago by jkarder

  • Status changed from assigned to accepted

comment:37 Changed 2 years ago by jkarder

  • Status changed from accepted to readytorelease

comment:38 in reply to: ↑ 34 Changed 2 years ago by 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.

comment:39 Changed 2 years ago by jkarder

  • Resolution set to done
  • Status changed from readytorelease to closed
Note: See TracTickets for help on using tickets.