SlicerMorph / SlicerScriptEditor

a simple programming editor for Slicer based on monaco
BSD 2-Clause "Simplified" License
5 stars 2 forks source link

Save As (scene save), tries to save the editor file as a txt file #44

Closed muratmaga closed 1 month ago

muratmaga commented 2 months ago

This is confusing, since this is not a text object. It should save as .py (or at least allow the user to specify the extension).

image

pieper commented 2 months ago

@oothomas - probably this can be addressed with the custom fileio plugin. Let me know if you hit a roadblock on that.

muratmaga commented 2 months ago

To be clear, custom file writer works for Export File in subject hierarchy but doesnt seem to be registered/recognized for Save as.

I believe @oothomas got stuck figuring out why.

pieper commented 2 months ago

It may be necessary to make a C++ node type to make all features work on nodes that contain scripts seamlessly. It's not a lot of work but it adds some complexity.

Probably we should come up with a priority ranking scheme for features and bugs vs the effort required and the benefit to users - we could use the labels feature of github to track them.

muratmaga commented 2 months ago

I agree a set of priorities is important, since this is quite a side project for us.

Embedding the script into the scene was the primary motivator of development of this extension. And currently that functionality does not function correctly (save as saves it as text node, export as saves is a py). I think we should probably fix that first, followed by better integration of sending code to python console.

Beyond that I am inclined to leave the feature additions to the community.

muratmaga commented 1 month ago

@pieper @oothomas any updates on this?

pieper commented 1 month ago

I haven't been able to spend any time on this. I'm hoping @oothomas has bandwidth to investigate along the lines discussed above.

muratmaga commented 1 month ago

t may be necessary to make a C++ node type to make all features wor

I think @oothomas needs some examples of this. I don't think he is familiar with this aspect of Slicer? Are there any examples he can use as guideline?

pieper commented 1 month ago

So far the editor has "hijacked" the text node with a flag (attribute) because it can do most of what we need. But if the behavior needs to be changed more fundamentally, then making a dedicated node type means you can have a dedicated storage node and other custom behavior. In the case of the Save dialog this would allow you to save to .py instead of text, but the actual subclasses wouldn't need to do much else. It would be great if this were possible in python but unless I missed something I think C++ is needed in the current architecture.

The DMRI module provides custom MRML nodes and could be used as an example, but a script node would be much simpler, basically just providing hooks.

Another option could be looking into creating a mechanism in the slicer core so that this save as function could be overridden in python. It may not need much change to make that work and it would be generally useful but it's probably more work than just following the existing pattern.

lassoan commented 1 month ago

I'll have a look if we can make it show up by using a file writer plugin.

lassoan commented 1 month ago

I've managed to read/write .py files using vtkMRMLTextStorageNode and custom reader and writer plugin. I had to force creation of storage node (otherwise short texts are saved into the scene), use canread/canwrite method variants that return confidence (not just a bool), and allow vtkMRMLTextStorageNode to handle all text files (small change in Slicer core).

There are two remaining issues:

I think the simplest would be to add a vtkMRMLPythonScriptStorageNode in C++ that only overrides the file extension related methods of the vtkMRMLTextStorageNode.

lassoan commented 1 month ago

We discussed this today with @pieper and concluded that making file extension configurable in the text storage node could be a good approach (it allows implementing this feature and could be potentially useful for supporting other text-based file formats in the future). I'll submit a pull request for this in Slicer soon and then update this extension to use this.

muratmaga commented 1 month ago

Thank you both! @lassoan you had concerns about the Extension name (to avoid confusion with Segment Editor). if they are still relevant, please feel free add to your PR.