Update Qt connection syntax in Inelastic Interfaces #38119
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adriazalvarez
added
Indirect/Inelastic
… it is changed to QComboBox::currentIndexChanged(int) in the affected instances
There was a problem hiding this comment.
I have tested all the interfaces, and they are working as expected. But there is some problem with Bayes and Stretch tabs. When I try to run them, they get stuck in the running state. This is not related to this PR because the issue happens in the nightly, too. Please check if you can replicate this issue on your side
|
Thanks for testing. |
|
I am using the same ones as in the test instructions. I think the issue might be occurring on my laptop because I am experiencing memory issues. I will attempt to take another look to see if there is a problem, but I am happy to approve now as this is not related to this pull request. |
MohamedAlmaki
previously approved these changes
Oct 11, 2024
qt/scientific_interfaces/Inelastic/Corrections/ApplyAbsorptionCorrections.cpp
Outdated
Show resolved
Hide resolved
robertapplin
approved these changes
Oct 14, 2024
robertapplin
deleted the
new_qt_connection_syntax_inelastic_interfaces
branch
mpatrou
pushed a commit
that referenced
this pull request
Nov 22, 2024
This is a squashed version of #38119 new signal/slot syntax changed on QENSFitting update signals/slots in processor and corrections interfaces new signal/slots on bayes fitting interface QComboBox::currentIndexChanged(QString &) was found to be deprecated, it is changed to QComboBox::currentIndexChanged(int) in the affected instances remove accidental includes
mpatrou
added a commit
that referenced
this pull request
Nov 25, 2024
Add tests for IqtPresenter tab #37901 Elwin allow processing ILL data #38033 Update Qt connection syntax in Inelastic Interfaces #38119 Update Moments tab instructions and add more tests #38274 Construct the model of OutputPlotOptions widget outside the Presenter class. #38289 Add identifier for output workspaces in Elwin Interface #37762 MVP for the ResNorm tab of the Inelastic Bayes Fitting interface #38357 Remove deprecated Calculate Paalman Pings tab from Inelastic Corrections #38391 Use std::pair instead of QPair in utility files #38396 Elwin Elastic line integration issue #38392 --------- Co-authored-by: adriazalvarez <adrian.diaz-alvarez@stfc.ac.uk> Co-authored-by: Applin <robert.applin@stfc.ac.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of work
Changes the signals and slots of the Inelastic interfaces from string-based to functor-based connections (getting rid of SIGNAL/SLOT macros).
One of the main advantages is that the connections are checked at compile-time, so we avoid the problem of missing connections in qt interfaces at runtime.
Additionally, syntax is probably more straightforward, as it explicitly states the object that sends/receives the signal.
On the flip side, the syntax for overloaded signals/slots (which there are a few) is a little bit more involved, as the function has to be cast with a pointer to member function. (qoverloaded classes could have been used here, but it is not available for qt 5.15)
Details here: https://wiki.qt.io/New_Signal_Slot_Syntax
Also, if the slots are expected to be called with a default argument, a lambda function shall be used.
This is first part of #37904 , indirect interface and spectroscopy widget libraries are also going to be updated soon.
Summary of work
Fixes #37904 (partially).
Further detail of work
To test:
Testing is straightforward, although probably tedious. I have tested it on MacOS and every GUI seems to be responding as expected.
https://developer.mantidproject.org/Testing/Inelastic/BayesFittingTests.html
https://developer.mantidproject.org/Testing/Inelastic/CorrectionsTests.html
https://developer.mantidproject.org/Testing/Inelastic/DataProcessorTests.html
https://developer.mantidproject.org/Testing/Inelastic/QENSFittingTests.html
Does not required release notes as it is an internal change
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.