Inria-Asclepios / medInria-public

Open-source part of the medInria software
https://med.inria.fr/
BSD 4-Clause "Original" or "Old" License
0 stars 9 forks source link

[Cropping] reset box if tlbx is switched to another & add reset button #826

Open mathildemerle opened 8 months ago

mathildemerle commented 8 months ago

This PR is about the Cropping bug in https://github.com/Inria-Asclepios/music/issues/1056

The problem was that there was no way to reset a Cropping box after the disappearance of the box, if the user moves it outside of the view for instance.

This PR adds a reset of the box if you switch to an other toolbox, change data, apply data (the new data is applied in the view). In pipeline it's possible to Reset the toolbox directly with the pipeline button.

Concerning the orientation problem on mac, i had it once and couldn't redo it, so i couldn't really work on that qt or vtk problem, but resetting the toolbox solves the problem.

:m:

fcollot commented 8 months ago

If you apply the crop, then move the box and reset, the box disappears and from then on apply causes "Cropping: This action failed (undefined error)" and will not work anymore (even when closing the view and adding new data).

mathildemerle commented 8 months ago

@fcollot i don't succeed to redo your error. Did you change the orientation or move the box without resetting, or do something in addition ? Indeed if i move the box outside the view and apply i've got the error (it does that even without this PR), but resetting works. I'll continue to test

mathildemerle commented 8 months ago

Are you talking about the new Reset button in the toolbox, or the Reset button in a the pipeline toolbox ?

fcollot commented 8 months ago

The toolbox button. On every data if I apply the crop, then move the cropping box and click reset, it causes the bug.

mathildemerle commented 8 months ago

It's interesting because Kévin had some disappearance of the box changing the orientation, that i had only once and could not reproduce. It can be the same underlying problem

fcollot commented 8 months ago

I don't think it's related. From looking at the code there are some issues I believe could cause this.

mathildemerle commented 8 months ago

(i just removed a duplicated line, render was done also in updateBorderWidgetIfVisible) If you see some problems do not hesitate to explain them, because i can't reproduce the bug

fcollot commented 8 months ago

(that render line was one of the things I was mentioning, but I wanted to do a few tests first) I still have this bug (for example when I move the box, then reset, then apply and reset again). I can reproduce this bug every time by simply doing a combination of move + reset -> apply -> reset (or a different order, as long as there is a reset after moving). Since it breaks the toolbox permanently (until closing the app) I can't validate this.

mathildemerle commented 8 months ago

Yes, however since Kévin had the same bug just changing the orientation, can you test with or without this PR? Because changing the orientation and resetting lead to the same updateBorderWidgetIfVisible() line, from reactToOrientationChange() or resetBoxPlace()

fcollot commented 8 months ago

Since it is not possible to reproduce my test without this PR I suppose you are talking about the pipeline reset button (which is entirely different since it destroys and recreates the toolbox and therefore everything inside it including the border widget). I just tested this multiple times, moving the box in different orientations, resetting, clicking next then coming back etc. without any bug.

mathildemerle commented 8 months ago

I was talking about the orientations in the "View settings" toolbox. Kévin changed the orientation of its view and the box disappeared, even without resetting the toolbox through the pipeline Reset button. That was the main problem i was trying to solve at the beginning, but since i did not have this bug on my mac, i wanted to give the opportunity to reset the box. In fact resetting the box in the toolbox in the workspace is needed when the user moves the box outside the view. However thx to your test it seems that it does not bring back to life a box if it disappeared completely (maybe vtkBorderWidget or vtkBorderRepresentation crashes, i'm not sure)

mathildemerle commented 8 months ago

I removed this one from the 4.0 milestone. It's a feature that could be interesting later