Skip to content

Safe flag#3910

Open
jellybean2004 wants to merge 6 commits into
mainfrom
safe_flag
Open

Safe flag#3910
jellybean2004 wants to merge 6 commits into
mainfrom
safe_flag

Conversation

@jellybean2004

Copy link
Copy Markdown
Member

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

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)
    • Wheels installer (GH artifact) has been tested (installed and worked)

Licensing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

@jellybean2004

Copy link
Copy Markdown
Member Author

Hi @pkienzle! Would you be happy to review this PR?

@pkienzle pkienzle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/sas/qtgui/Plotting/SlicerModel.py Outdated
Comment thread src/sas/qtgui/Plotting/Slicers/MultiSlicerBase.py Outdated
@jellybean2004

Copy link
Copy Markdown
Member Author

Hi @pkienzle! Thank you for your review, and sorry for taking time to get back on this.

I have implemented the changes you suggested, which include using try-finally blocks with blockSignals. Please let me know what you think.

@pkienzle

pkienzle commented May 5, 2026

Copy link
Copy Markdown
Contributor

Slicer is not updating from dialog.

Try showing a box slicer and typing values for parameters into the dialog. I find that the box does not update until the cursor hovers on one of the slicer handles. Worse, it's not where the handle is displayed, but rather where it will appear after it is moved. To see this, move the box out in qx then set the qx location of the box center. Move the cursor horizontally until the box suddenly springs to the new position. This occurs at the new qx position.

I'm testing on a mac with pyside 6.9.1

@jellybean2004

Copy link
Copy Markdown
Member Author

Slicer is not updating from dialog.

Try showing a box slicer and typing values for parameters into the dialog. I find that the box does not update until the cursor hovers on one of the slicer handles. Worse, it's not where the handle is displayed, but rather where it will appear after it is moved. To see this, move the box out in qx then set the qx location of the box center. Move the cursor horizontally until the box suddenly springs to the new position. This occurs at the new qx position.

I'm testing on a mac with pyside 6.9.1

I could replicate the problem on Windows 11 -- it seems to be only in box sum, with all other slicers working fine. But I found the same issue on main, so it is not introduced by changes here.

I will see if I can find a fix for this regardless. For now, I will raise an issue for this.

@pkienzle

Copy link
Copy Markdown
Contributor

SasView is gaslighting me. I was about to report that tabbing to the next field after changing a value in the slicer dialog does not update the slicer. I went to confirm that clicking in another field also does not update the slicer (a check I'm sure I've done before, but I wanted to confirm it before creating a new ticket), and suddenly both tabbing and clicking are working.

I'm wondering if the signal is somehow getting stuck blocked for some time? I haven't been able to reproduce this problem.

While trying to clear the slicers and start afresh I got this error:

11:22:15 - ERROR: Traceback (most recent call last):
File ".../sasview/src/sas/qtgui/Plotting/Plotter2D.py", line 292, in onClearSlicer
    self.slicer.clear()
    ~~~~~~~~~~~~~~~~~^^
File ".../sasview/src/sas/qtgui/Plotting/Slicers/BoxSum.py", line 182, in clear
    self.widget.closeWidgetSignal.emit()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
RuntimeError: Signal source has been deleted

Trying again from restart, updates work and the slicer can be deleted.

And again, closing the dialog before clearing slicers raises the above error. It seems to be reproducible:

  • restart sasview app
  • open a 2D dataset
  • plot [create new] button
  • Right-Click on graph > Slicers > Box Sum
  • Close slicer dialog
  • Right-Click on graphs > Slicers > Clear slicers

@pkienzle pkienzle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to unapprove

@jellybean2004

Copy link
Copy Markdown
Member Author

Hi @pkienzle! The issue you reported should now be resolved from #3970.

If a slicer plot, or the dialog box in the case of box sum, is closed before clearing the slicer, the slicer gets auto-deleted.

@pkienzle

Copy link
Copy Markdown
Contributor

@jellybean2004 What's with the force-push?

It leads to surprises when pulling your PRs to check the latest version.

Are you rebasing on main?

@pkienzle pkienzle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spurious errors from test for number of named colormaps unrelated to this PR (fixed in #4000).

Code runs, with updates tied between the parameter dialog and the interactor.

@pkienzle

Copy link
Copy Markdown
Contributor

Ubuntu latest tests were stuck on install for over 2 hrs. Hopefully this was a hiccup in the CI infrastructure and not a recurring problem.

@jellybean2004

Copy link
Copy Markdown
Member Author

@pkienzle Yes, I've been rebasing on main and git push force-with-lease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor update_model handling in slicer models

2 participants