populse / populse_mia

Multiparametric Image Analysis
Other
10 stars 9 forks source link

[Undo / Redo] Same issue as issue#277 with Coregister node from spm #279

Closed ghost closed 2 years ago

ghost commented 2 years ago

Mia crashes unexpectedly just as the operate mode issue#277 of Coregister node from spm. Indeed, from Pipeline Manager:

-1) Drag and drop Coregister node(mia_processes->bricks->spm) from packages from left tab to New Pipeline window -2) Right click on coregister_1 in New Pipeline window -3) Export all unconnected plugs -4) Execute a undo with Ctrl+Z without clicking anywhere.

From now you can repeat actions from 1) to 4). If so, when you make step 4) for the second time Mia crashes.

We get the following generated error for both cases:

Traceback (most recent call last):
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/main_window.py", line 1567, in undo
    self.pipeline_manager.undo()
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_manager_tab.py", line 2458, in undo
    c_e.update_plug_value(node_name, old_value, plug_name,
  File "/casa/home/DEV/populse_dev/populse_mia/python/populse_mia/user_interface/pipeline_manager/pipeline_editor.py", line 1118, in update_plug_value
    old_value = self.scene.pipeline.nodes[node_name].get_plug_value(
  File "/casa/home/DEV/populse_dev/capsul/capsul/pipeline/pipeline_nodes.py", line 765, in get_plug_value
    if not isinstance(self.get_trait(plug_name).handler,
AttributeError: 'NoneType' object has no attribute 'handler'

image

From this try undo two times and see mia crashing.

EDIT It seems to me that the minimum procedure to reproduce is not strictly the right one. On my station the procedure that produces the bug is:

-1) Drag and drop Coregister node (mia_processes->bricks->spm) from packages from left tab to New Pipeline window -2) Right click on coregister_1 in New Pipeline window -3) Export all unconnected plugs -4) Execute an undo with Ctrl+Z without clicking anywhere, twice, to return to an empty editor. -5) Repeat steps from 1) to 4), at the second Ctrl+Z, Mia crash

servoz commented 2 years ago

I'm not reproducing the issue with the minimal procedure of the first post (@YannNdr, try to be very meticulous when giving a procedure to reproduce a bug, please). I will edit the first post with the correct procedure (which I think, please, @YannNdr can you validate this edit?).

servoz commented 2 years ago

I have just tested the procedure indicated in the EDIT of the first post ... Unfortunately the fix for #277 does not fix this ticket ... We still have to work on this ticket (#279) ...

servoz commented 2 years ago

@YannNdr, please synchronise regularly #279 and master. #279 is 45 commits behind master and i think your last commit in #279 was already done in master ...

ghost commented 2 years ago

@servoz , agreed, sorry for that. Considering the last commit in #279, it's probably the same title used but the approach is different I think. I'll check that.

servoz commented 2 years ago

no, no, we can't work on a branch that has diverged so much, this is a general remark that has nothing to do with the current tickets...

Regarding the tickets, your last modification:

if node_name not in ('inputs', 'outputs') :
            return

(the other one was already done in master), I wonder if it can't lead to a regression (if the node is not a pipeline node there will be no notification...). I haven't had the time to think about a concrete case yet, I will do it as soon as possible

servoz commented 2 years ago

I just synchronised issue#279 with master. Don't forget to pull issue#279 quickly.

ghost commented 2 years ago

no, no, we can't work on a branch that has diverged so much, this is a general remark that has nothing to do with the current tickets...

Regarding the tickets, your last modification:

if node_name not in ('inputs', 'outputs') :
            return

(the other one was already done in master), I wonder if it can't lead to a regression (if the node is not a pipeline node there will be no notification...). I haven't had the time to think about a concrete case yet, I will do it as soon as possible

I see the point, I'll find out an other way.

I just synchronised issue#279 with master. Don't forget to pull issue#279 quickly.

Thank you, just pulled #279

servoz commented 2 years ago

As I was wondering, this commit allows to fix this ticket but ... introduces a new regression...

Minimum procedure to reproduce the regression:

0) On the current issue#279 branch 1) Drag and drop the Coregister node (mia_processes->bricks->spm) from the left tab to the New Pipeline window 2) Right click on the coregister_1 node in the New Pipeline window 3) Export the unconnected mandatory files 4) Left-click on the coregister_1 node, then change the value of the write_interp form from 4 to 3. 5) Left click on the New Pipeline window and then press Ctrl+Z => the plug value does not return to 4!

This is due to the change (we lose notification for a node that is not a pipeline node):

if node_name not in ('inputs', 'outputs') :
            return

Finally, I propose a fix directly in master (c6c813b). I think this ticket can now be closed and we can delete the issue#279 branch.