Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove QObjects from ALCFitting Presenter and Model #38211

Merged

Conversation

cailafinn
Copy link
Contributor

Description of work

Summary of work

Stops the presenter and the model from being QObjects, replacing their signals and slots with functionality based on the observer pattern where the presenter (as an observer) subscribes to the view and the model and is updated on their progress.

As part of this, the view and model no longer directly communicate, making this compliant with MVP principles.

Fixes #38007

Further detail of work

Tests have been updated with new mocks to test the implemented functionality independently of any changes to the other parts of the MVP structure.

To test:

  1. Run through the tests here: https://developer.mantidproject.org/Testing/MuonAnalysis_test_guides/Muon_ALC.html#alc-peak-fitting

This does not require release notes because no user-facing functionality should have changed.


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.

@cailafinn cailafinn added this to the Release 6.12 milestone Oct 17, 2024
@cailafinn cailafinn force-pushed the 38007_fitting_presenter_qobject branch from 3c3d10e to 16dbae7 Compare October 17, 2024 17:08
@cailafinn cailafinn force-pushed the 38007_fitting_presenter_qobject branch from 16dbae7 to d069ad0 Compare October 28, 2024 15:00
@cailafinn cailafinn marked this pull request as ready for review October 30, 2024 10:47
@adriazalvarez adriazalvarez self-assigned this Oct 30, 2024
@adriazalvarez adriazalvarez added Muon Issues and pull requests related to muons ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS GUI Issues and pull requests specific to the Mantid Workbench GUI. labels Oct 30, 2024
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

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

Trying to do the manual testing, there are two problems:
On the baseline fitting step, there are three legends on the plot after fitting:
Screenshot 2024-10-30 150329
And the data for the peak is not carried over to the peak fitting tab (it is empty):
image

@cailafinn
Copy link
Contributor Author

Thanks for the review @adriazalvarez
Would you be able to provide the data you used for the test? I struggled to find it in any of the Usage/Sample data sets.

@adriazalvarez
Copy link
Contributor

It was not local data.I followed the first test (https://developer.mantidproject.org/Testing/MuonAnalysis_test_guides/Muon_ALC.html#alc-peak-fitting) of the manual testing instructions (run numbers 81061-142).

@cailafinn
Copy link
Contributor Author

And the data for the peak is not carried over to the peak fitting tab (it is empty)

This actually seems to be an issue on main, but I'll fix it as part of this PR.

Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

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

Tested the interface and everything works correctly on the peak fitting tab.
It's a nice implementation of the MVP pattern with subscribers and is a clean and clear refactorization. I have found very small issues that in some cases are just questions. But it looks good otherwise.
Also, I have checked(I didn't when I tested initially) that the recent refactorization for the MVP training of the baseline presenter was already on the code, and the issue I experienced initially was a bug with that, but thanks a lot for fixing it anyway!

@github-actions github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Nov 14, 2024
Copy link

👋 Hi, @cailafinn,

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

@cailafinn cailafinn force-pushed the 38007_fitting_presenter_qobject branch from d228d91 to a21f26c Compare November 14, 2024 12:37
@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Nov 14, 2024
Copy link
Contributor

@adriazalvarez adriazalvarez left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, peak fitting is working correctly and the interface organization is much improved.

@robertapplin robertapplin self-assigned this Nov 15, 2024
@robertapplin robertapplin merged commit 7be204a into mantidproject:main Nov 15, 2024
10 checks passed
peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Dec 5, 2024
)

* Remove Qt elements from model

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>
Co-authored-by: James Crake-Merani <[email protected]>

* Fix model tests

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>

* Strip Qt from presenter

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>

* Remove commented methods

RE mantidproject#38007

* Use mondern mocking standard

RE mantidproject#38007

* Fix CppCheck issues

RE mantidproject#38007

* Add missing virtual destructor

RE mantidproject#38007

* Update fitting tab from baseline tab

RE mantidproject#38007

* Add subscriber in tests

RE mantidproject#38007

* Remove unnecessary comments and includes

RE mantidproject#38007

* Modernise connections and pointers

RE mantidproject#38007

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>
Co-authored-by: James Crake-Merani <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
peterfpeterson pushed a commit to peterfpeterson/mantid that referenced this pull request Jan 16, 2025
This is a squashed version of mantidproject#38211

Remove Qt elements from model

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>
Co-authored-by: James Crake-Merani <[email protected]>

Fix model tests

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>

Strip Qt from presenter

RE mantidproject#38007

Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]>

Remove commented methods

RE mantidproject#38007

Use mondern mocking standard

RE mantidproject#38007

Fix CppCheck issues

RE mantidproject#38007

Add missing virtual destructor

RE mantidproject#38007

Update fitting tab from baseline tab

RE mantidproject#38007

Add subscriber in tests

RE mantidproject#38007

Remove unnecessary comments and includes

RE mantidproject#38007

Modernise connections and pointers

RE mantidproject#38007

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues and pull requests specific to the Mantid Workbench GUI. ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS Muon Issues and pull requests related to muons
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

ALCPeakFittingPresenter inherits from a QObject
3 participants