Add editing flag to ScriptParser

[rdswift]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences: This adds an editing flag to the ScriptParser class to indicate that the script is currently being edited.

Problem

There may be occasions when a script function should not execute some of its code during the development of a script containing the function. An example is https://github.com/rdswift/picard-plugin-file-writer which would actually write to the file when the function is entered into the script under development.

• JIRA ticket (optional): PICARD-XXX

Solution

Add an editing boolean attribute (defaulting to False) to the ScriptParser class that can be checked within the script function. Set this value to true for the parsers defined during tagging or file naming script development. This new attribute is not checked in any internal script function definitions, so has no effect on existing code.

Changed to an alternate approach, which is actually much simpler. This provides an is_editing_scripts() method to MainWindow which returns True if either the Options window or the File Naming Script Editor window is open. It can be checked easily from the api using api.tagger.window.is_editing_scripts().

Action

Additional actions required:

• Update Picard documentation (please include a reference to this PR)
• Other (please specify below)

[zas]

This new attribute is not checked in any internal script function definitions, so has no effect on existing code.

When it is checked then?

I'm not sure to understand why this is needed.

There may be occasions when a script function should not execute some of its code during the development of a script containing the function.

Which occasions? Can you clarify?

[phw]

When it is checked then?

I'm not sure to understand why this is needed.

There is no built-in function currently, that would need it.

The point here is that there might be scripts with side-effects. The example rdswift gave is https://github.com/rdswift/picard-plugin-file-writer, which actually adds scripting functions that write to files on disk. You'd expect this writing to happen if the script is regularly executed. But during editing the script might get executed rather randomly (for generating the previous) with testing data.

So I understand that is is useful for that scripting function is able to tell whether is is being called in a editing context or executed for a real use case.

The PR here only adds this indication, but since there is no built-in function with such requirements it is otherwise not being used by Picard code.

On a side note however I also generally think that scripting functions should ideally not have such side effects, and I would be against any proposal to add such functions to Picard or any official plugin. Right now scripts can change metadata and the scripting returns a string that is being used for filenames. Applying the changes resulting from this (saving tags, generating filenames) is then the responsibility of Picard, which can also ensure any errors and security implications are handled.

The File Writer plugin is very powerful, but this power also brings a lot of potential of things going wrong. From accidental damages to files up to serious security implications.

[phw]

Why is editing=True in this case? Isn't this the actual case of executing the naming script, and hence the functions are expected to run normally?

[rdswift]

The reason is that it is called when updating the examples in the file naming script editor.

[phw]

I see. But then it probably needs some way to set the editable flag when calling script to filename, because it is also used for real renaming

[rdswift]

But it still does the renaming because none of the internal functions look at the new attribute (unless I'm missing something).

[zas]

@phw thanks for the explanation.

I think we're going in the wrong direction here, script functions shouldn't have side-effects (and certainly not writing files, creating directories). Also adding code to handle this looks wrong to me.

It will lead to infinite complications.

About "editing context" I think it makes sense somehow but it should be made much more generic, so a plugin (or Picard code) actually check if we are in editing context, it means we should never execute actual code of a script function if we are in editing context, but what should we do instead? How to provide feedback to user so he knows that the function "would have been executed", it should at least log something.

As is, I'm against merging this PR as it seems to me that's adding code to solve one plugin issue, which implements something that isn't really expected for a script function.

Or at least we need more thoughts about it and its implications.

[rdswift]

I figured this one would be controversial.

I guess the general consensus is that I should kill my File Writer plugin and remove all traces of it from GitHub. That's unfortunate because it can provide valuable input for post processing scripts. It does indeed introduce some powerful functionality, but I understand that it could result in issues if used improperly.

[zas]

I figured this one would be controversial.

I guess the general consensus is that I should kill my File Writer plugin and remove all traces of it from GitHub. That's unfortunate because it can provide valuable input for post processing scripts. It does indeed introduce some powerful functionality, but I understand that it could result in issues if used improperly.

I don't think that a bad idea, I think that's a bad approach.

First, I would go for something much more generic:

I think we could instead introduce a call('function',args), that would be more generic than file_write().
It wouldn't call any function, but only those registered as script call function.
Any script call function would have a parameter (editing, dryrun, dunno?) that would be passed by Picard.

In the file write example we would have: call('plugin.name.file_write', .....) in the script, and Picard would actually call the function if it exists and was registered as a callable script function.

[rdswift]

Changed to an alternate approach, which is actually much simpler. This provides an is_editing_scripts() method to MainWindow which returns True if either the Options window or the File Naming Script Editor window is open. It can be checked easily from the api using api.tagger.window.is_editing_scripts().