Skip to content

fixed disconnect logic with the proper PySide6 signal checking method.#3771

Merged
wpotrzebowski merged 1 commit intomainfrom
3205-remove-obsolete-logic-from-constraintwidget
Nov 23, 2025
Merged

fixed disconnect logic with the proper PySide6 signal checking method.#3771
wpotrzebowski merged 1 commit intomainfrom
3205-remove-obsolete-logic-from-constraintwidget

Conversation

@rozyczko
Copy link
Copy Markdown
Member

@rozyczko rozyczko commented Nov 16, 2025

Description

Before: The code used a try/except approach to blindly attempt disconnecting signals, catching RuntimeError exceptions when signals weren't connected. We tried three ways to check signal connection status, which didn't work with Qt 6.2.4.

After: The code now uses the proper isSignalConnected() method with QMetaMethod.fromSignal(), which is the official and clean way to check if a signal is connected before attempting to disconnect it.

The proper usage pattern is

signal_method = QtCore.QMetaMethod.fromSignal(obj.someSignal)
if obj.isSignalConnected(signal_method):
    obj.someSignal.disconnect()

as per https://doc.qt.io/qtforpython-6/PySide6/QtCore/QObject.html#PySide6.QtCore.QObject.isSignalConnected

Fixes #3205

How Has This Been Tested?

Local testing

Review Checklist:

Documentation (check at least one)

  • [X ] 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)

wpotrzebowski
wpotrzebowski reviewed Nov 17, 2025
View reviewed changes
Comment thread src/sas/qtgui/Perspectives/Fitting/ConstraintWidget.py
@wpotrzebowski
Copy link
Copy Markdown
Contributor

wpotrzebowski commented Nov 17, 2025

It is bit difficult to test it. It seems to work e.g. no errors when closing windows

I am getting
07:53:33 - ERROR: Traceback (most recent call last): File "sas/qtgui/Perspectives/Fitting/ConstraintWidget.py", line 1133, in showMultiConstraint File "sas/qtgui/Perspectives/Fitting/ComplexConstraint.py", line 40, in __init__ File "sas/qtgui/Perspectives/Fitting/ComplexConstraint.py", line 79, in setupWidgets File "sas/qtgui/Perspectives/Fitting/ComplexConstraint.py", line 105, in setupParamWidgets IndexError: list index out of range
when clicking on Add Constraints button but it seems to be unrelated to that

@rozyczko
Copy link
Copy Markdown
Member Author

rozyczko commented Nov 17, 2025

Thanks @wpotrzebowski . Can you please tell me how you got this exception from Add Constraints?
I'd like to make sure this change didn't introduce it.

@wpotrzebowski
Copy link
Copy Markdown
Contributor

wpotrzebowski commented Nov 17, 2025

Thanks @wpotrzebowski . Can you please tell me how you got this exception from Add Constraints? I'd like to make sure this change didn't introduce it.

It simply happens when trying to "Add Constraints"

@rozyczko
Copy link
Copy Markdown
Member Author

rozyczko commented Nov 18, 2025

Thanks @wpotrzebowski . Can you please tell me how you got this exception from Add Constraints? I'd like to make sure this change didn't introduce it.

It simply happens when trying to "Add Constraints"

I have downloaded the installer and tested the functionality - the constraints work well. Can you please test the installer?

@wpotrzebowski
Copy link
Copy Markdown
Contributor

wpotrzebowski commented Nov 18, 2025

Ok, I see what I did... I didn't choose any model and that's when it fails. When models are selected, it works as expected. I guess it is a new bug?

@rozyczko
Copy link
Copy Markdown
Member Author

rozyczko commented Nov 19, 2025

Ok, I see what I did... I didn't choose any model and that's when it fails. When models are selected, it works as expected. I guess it is a new bug?

Please show how you got this error - I can't reproduce the issue in this or in other branches.

@wpotrzebowski
Copy link
Copy Markdown
Contributor

wpotrzebowski commented Nov 19, 2025

Hope, these screenshots help:

Screenshot 2025-11-19 at 14 33 57 Screenshot 2025-11-19 at 14 33 44

@krzywon
Copy link
Copy Markdown
Contributor

krzywon commented Nov 19, 2025

I'm seeing the same error as @wpotrzebowski, but I tested the procedure outlined below in v6.1.1 and see the same error there as well. I think this is an existing, but previously uncaught, issue, so it shouldn't hold this PR, IMO.

Steps to reproduce:

  1. Open the Const/Simul Fit Window
  2. Load at least two data sets and send both to fitting.
  3. The Add constraints is now active in the Const/Simul Window. Press it!
  4. Error

It seems like the constrained fit panel is enabling the constraint button before a model is selected in the fit pages.

@wpotrzebowski
Copy link
Copy Markdown
Contributor

wpotrzebowski commented Nov 20, 2025

I suppose we can merge this one and create a separate issue for the add constraints error?

@rozyczko rozyczko mentioned this pull request Nov 22, 2025
Closed
@rozyczko
Copy link
Copy Markdown
Member Author

rozyczko commented Nov 22, 2025 •
edited
Loading

I suppose we can merge this one and create a separate issue for the add constraints error?

#3785

Can someone please accept this PR then so I merge it?

@wpotrzebowski wpotrzebowski merged commit 1414a3a into main Nov 23, 2025
30 checks passed
@wpotrzebowski wpotrzebowski deleted the 3205-remove-obsolete-logic-from-constraintwidget branch November 23, 2025 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wpotrzebowski wpotrzebowski wpotrzebowski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove obsolete logic from ConstraintWidget

3 participants

@rozyczko @wpotrzebowski @krzywon