IAddWorkspaceDialog moved to a common location and no longer a qobject #36624

robertapplin · 2024-01-08T17:12:44Z

Description of work

This PR moves the IAddWorkspaceDialog class to the MantidQtWidgetsCommon library, and also combines the functionality of the AddWorkspaceDialog class with the IndirectAddWorkspaceDialog class because they are essentially the same thing. This has meant I could remove the IndirectAddWorkspaceDialog class and just use the standard AddWorkspaceDialog class from the MantidQtWidgetsCommon library.

This PR also makes sure the IAddWorkspaceDialog class is no longer a QObject.

Fixes #36565
Fixes #36756

To test:

To test Fit Script Generator, follow the instructions on this page:
https://docs.mantidproject.org/nightly/interfaces/general/Fit%20Script%20Generator.html

To test Inelastic Data Analysis follow these instructions
https://developer.mantidproject.org/Testing/Inelastic/DataAnalysisTests.html

This does not require release notes because this is not a new feature or bug fix

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

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there is GUI work does it follow the GUI standards?
If there are changes in the release notes then do they describe the changes appropriately?
Do the release notes conform to the release notes guide?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will 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.

github-actions · 2024-01-09T07:55:57Z

👋 Hi, @robertapplin,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

github-actions · 2024-01-15T11:28:11Z

👋 Hi, @robertapplin,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

adriazalvarez

I have followed the test instructions and everything works fine, also some sneaky qt warnings that these FqFitWorkspaceDialogs were producing are not there anymore. this is another great step on improving quality of life for developers of indirect/inelastic. Thank you.
I have minor comments and a question and then it's good to approve.

qt/scientific_interfaces/Inelastic/Common/IAddWorkspaceDialog.h

qt/scientific_interfaces/Inelastic/Analysis/IndirectFitDataPresenter.h

adriazalvarez · 2024-01-17T11:51:37Z

qt/scientific_interfaces/Inelastic/Analysis/ConvFitDataView.cpp

+  dialog->updateSelectedSpectra();
+  dialog->show();
+
+  m_addWorkspaceDialog = dialog;


This is just a question: I know lifetime of dialog is handled by qt classes, but seems you can create as many workspacedialogs as you want and the member m_addWorkspaceDialog would be pointing to the last one created. Can't that be a problem if multiple dialog instances are open at the same time and you try to add data from, say, the first opened workspaceDialog?

Good spot, you are right, this will cause an issue. I haven't fixed this yet in the latest commits - I will fix it when I get back from A/L, so lets keep this PR open until then

This is also a problem on main and release-next, so I have opened an issue #36756 that can be used to track the issue for the current release. This PR fixes the problem, so we should maybe consider for this to go into release-next

github-actions · 2024-01-18T08:06:33Z

👋 Hi, @robertapplin,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

…face library

qt/scientific_interfaces/Inelastic/Analysis/ConvFitAddWorkspaceDialog.cpp

adriazalvarez

I'm testing it. But the dialogs for I(q,t) and MSD fit tabs of Data Analysis don't work. You see opening them in a blink and closing inmediately again. Can you check if you see the same?

robertapplin · 2024-01-29T13:06:13Z

I'm testing it. But the dialogs for I(q,t) and MSD fit tabs of Data Analysis don't work. You see opening them in a blink and closing inmediately again. Can you check if you see the same?

🤦 Yes I get the same, it should be fixed now. Thanks!

adriazalvarez

Thanks for solving it quickly. It looks good and it also fixes #36710 .
It's a nice refactoring of workspace dialogs, happy to approve it.

robertapplin added Maintenance Unassigned issues to be addressed in the next maintenance period. ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS Inelastic labels Jan 8, 2024

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 9, 2024

robertapplin added this to the Release 6.9 milestone Jan 9, 2024

robertapplin mentioned this pull request Jan 11, 2024

IndirectFitDataPresenter is no longer a QObject #36564

Merged

robertapplin force-pushed the 36565-iadd-workspace-dialog-not-qobject branch from cd0c7d2 to 185b908 Compare January 11, 2024 14:33

robertapplin removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 11, 2024

robertapplin marked this pull request as ready for review January 12, 2024 15:33

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 15, 2024

robertapplin force-pushed the 36565-iadd-workspace-dialog-not-qobject branch from 8364c6d to f2490d9 Compare January 15, 2024 11:54

robertapplin removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 15, 2024

adriazalvarez self-assigned this Jan 16, 2024

adriazalvarez requested changes Jan 17, 2024

View reviewed changes

github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 18, 2024

robertapplin modified the milestones: Release 6.9, Release 6.10 Jan 18, 2024

robertapplin force-pushed the 36565-iadd-workspace-dialog-not-qobject branch from f2490d9 to 3d05620 Compare January 19, 2024 16:06

robertapplin removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 19, 2024

robertapplin mentioned this pull request Jan 29, 2024

Missing signal warning on Data Analysis Interface. #36710

Closed

robertapplin modified the milestones: Release 6.10, Release 6.9 Jan 29, 2024

robertapplin changed the base branch from main to release-next January 29, 2024 09:14

robertapplin added 4 commits January 29, 2024 09:34

Remove QDialog dependency from IAddWorkspaceDialog

d30e65d

Move IndirectAddWorkspaceDialog to MantidQtWidgets Common

dbb91ac

Remove old AddWorkspaceDialog files

80edfd7

Rename IndirectAddWorkspaceDialog to AddWorkspaceDialog

f09cc72

robertapplin added 10 commits January 29, 2024 09:35

Update FSG interface docs

f79871d

Change names of Dialog window titles

f383b26

Only add workspace index in FSG if it exists

5e65a19

Fix cppcheck warnings

c2c6a9d

Move interface classes to INC_FILES section

015ef82

Remove unused IAddWorkspaceDialog file

0dc171c

Use consistent qualification of AddWorkspaceDialog in Inelastic inter…

064c230

…face library

Do not hold pointer to dialog which can become invalidated

1dfba42

Do not hold raw pointer to dialog in Elwin tab

6c7dbce

Update mock objects and test files

0154344

robertapplin force-pushed the 36565-iadd-workspace-dialog-not-qobject branch from 212ae17 to 0154344 Compare January 29, 2024 09:35

robertapplin requested a review from adriazalvarez January 29, 2024 11:35

adriazalvarez reviewed Jan 29, 2024

View reviewed changes

qt/scientific_interfaces/Inelastic/Analysis/ConvFitAddWorkspaceDialog.cpp Show resolved Hide resolved

adriazalvarez reviewed Jan 29, 2024

View reviewed changes

Use raw pointer for creating add workspace dialog

2855541

adriazalvarez approved these changes Jan 29, 2024

View reviewed changes

SilkeSchomann merged commit 1630012 into mantidproject:release-next Jan 30, 2024
9 checks passed

github-actions bot mentioned this pull request Jan 30, 2024

Modify step calculation to give exact result in DiffractionFocussing #32767

Merged

This was referenced Jan 30, 2024

Access Violation when opening and using multiple AddWorkspaceDialog's #36756

Closed

IAddWorkspaceDialog is a QDialog #36565

Closed

robertapplin added Indirect/Inelastic Issues and pull requests related to indirect or inelastic and removed Inelastic labels Feb 6, 2024

robertapplin deleted the 36565-iadd-workspace-dialog-not-qobject branch May 17, 2024 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IAddWorkspaceDialog moved to a common location and no longer a qobject #36624

IAddWorkspaceDialog moved to a common location and no longer a qobject #36624

robertapplin commented Jan 8, 2024 •

edited

Loading

github-actions bot commented Jan 9, 2024

github-actions bot commented Jan 15, 2024

adriazalvarez left a comment

adriazalvarez Jan 17, 2024

robertapplin Jan 19, 2024

robertapplin Jan 29, 2024 •

edited

Loading

adriazalvarez Jan 29, 2024

github-actions bot commented Jan 18, 2024

adriazalvarez left a comment

robertapplin commented Jan 29, 2024

adriazalvarez left a comment

IAddWorkspaceDialog moved to a common location and no longer a qobject #36624

IAddWorkspaceDialog moved to a common location and no longer a qobject #36624

Conversation

robertapplin commented Jan 8, 2024 • edited Loading

Description of work

To test:

Reviewer

Code Review

Functional Tests

Gatekeeper

github-actions bot commented Jan 9, 2024

github-actions bot commented Jan 15, 2024

adriazalvarez left a comment

Choose a reason for hiding this comment

adriazalvarez Jan 17, 2024

Choose a reason for hiding this comment

robertapplin Jan 19, 2024

Choose a reason for hiding this comment

robertapplin Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

adriazalvarez Jan 29, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2024

adriazalvarez left a comment

Choose a reason for hiding this comment

robertapplin commented Jan 29, 2024

adriazalvarez left a comment

Choose a reason for hiding this comment

robertapplin commented Jan 8, 2024 •

edited

Loading

robertapplin Jan 29, 2024 •

edited

Loading