Conversation
|
Hi @pkienzle! Would you be happy to review this PR? |
pkienzle
left a comment
pkienzle
left a comment
There was a problem hiding this comment.
I think there is a simpler way to do things. The problem we are trying to avoid is that setting the value in the model parameter box from the interactor triggers the same callback as typing the value in the box.
Instead of using the update_model flag we may be able to block the signal in SlicerModel.setModelFromParams using blocked = self._model.blockSignals(True) in a try-block with self._model.blockSignals(blocked) in the finally-block.
blockSignals is already being used in MultiSlicerBase._synchronized_movend, though without the try...finally structure. This can probably be removed if we block signals in SlicerModel.setModelFromParams.
|
|
||
|
|
||
| @contextmanager | ||
| def temporary_flag(obj, attribute_name, value): |
There was a problem hiding this comment.
This is more generic than a flag setter. How about pushattr, which follows the getattr/setattr naming convention?
This code won't work correctly if it receives the equivalent of a keyboard interrupt. For that you would need to move the setattr inside the try block.
It also assumes that the attribute exists. To make it equivalent to setattr (not necessary in this case) you would need to something like old = getattr(obj, attr, _sentinel) where _sentinel = object(). Then in the finally block you can call delattr instead of setattr if old is _sentinel and hasattr(obj, attr).
| slicer.update_model = old_update_model | ||
| # Temporarily enable model updates for this slicer. | ||
| with temporary_flag(slicer, "update_model", True): | ||
| with warnings.catch_warnings(): |
There was a problem hiding this comment.
You can use , to combine context managers without adding another indent level. Though with black formatting this might become uglier than nested with statements.
2 participants
Description
Fixes #1576 by implementing a shared context manager that restores the previous flag value safely, wires it into current call sites, and adds tests to prove restoration on exceptions.
How Has This Been Tested?
Ran the new tests and tested manually.
Review Checklist:
Documentation
Installers
Licensing (untick if necessary)