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.

Sign up for GitHub

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

Jump to bottom

Extracted Logic in Pairing Table View to Presenter #38213

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Extracted Logic in Pairing Table View to Presenter #38213

wants to merge 7 commits into from

Conversation

yusufjimoh
Copy link
Contributor

@yusufjimoh yusufjimoh commented Oct 17, 2024 •
edited
Loading

Description of work

  • extracted business logic from the pairing table view to the presenter.
  • cleaned up some code in the view and presenter.
  • updated unit tests to reflect changes in the view and presenter

Fixes #38105

To test:

  • testing instructions are included in the associated issue
  • code should pass unit tests

This does not require release notes because it involves code restructuring, no new functionality has been added

@yusufjimoh yusufjimoh linked an issue Oct 17, 2024 that may be closed by this pull request
Logic in the PairingTableView #38105
Open
@yusufjimoh yusufjimoh added ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS Muon Issues and pull requests related to muons Maintenance Unassigned issues to be addressed in the next maintenance period. labels Oct 17, 2024
@yusufjimoh yusufjimoh added this to the Release 6.12 milestone Oct 17, 2024
@yusufjimoh yusufjimoh marked this pull request as ready for review October 18, 2024 08:48
@adriazalvarez adriazalvarez self-assigned this Oct 18, 2024
@yusufjimoh
Updated Failing Tests
b16a449 
- Updated Failing Tests
- Created Getters for Properties in PairingTableView so the properties are accesed through the getters in the PairingTablePresenter
adriazalvarez
adriazalvarez requested changes Oct 21, 2024
View reviewed changes
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 effort on moving the logic out of the view class. I know this view is not easy to refactor ! Generally the view seems more neat and decluttered, although I have found some moved functions that maybe belong in the view, and we could subscribe the presenter to the view to avoid connecting signals/slots on the presenter and use notifiers instead to call the relevant functions on the presenter.

Also, when testing I have found this error when i click on Load current run :

image

...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Show resolved Hide resolved
...nterfaces/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_constants.py Outdated Show resolved Hide resolved
@yusufjimoh yusufjimoh force-pushed the 38105-logic-in-the-pairingtableview branch from dfab969 to 9974af2 Compare October 24, 2024 11:16
adriazalvarez
adriazalvarez requested changes Oct 29, 2024
View reviewed changes
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.

Except for a typo that is creating a runtime error, I have followed the manual testing instructions without any functional issue on the pair table widget. Thanks for the good work on this refactoring.
I have added some additional comments on small things that I've noticed during review.

...nterfaces/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_constants.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...erfaces/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_view.py Outdated Show resolved Hide resolved
qt/python/mantidqtinterfaces/test/Muon/grouping_tab/pairing_table_group_selector_test.py Outdated Show resolved Hide resolved
qt/python/mantidqtinterfaces/test/Muon/grouping_tab/pairing_table_group_selector_test.py Outdated Show resolved Hide resolved
@yusufjimoh
Apply Suggested Changes from code review to PairingTableWidget View a…
dac5e0d 
…nd Presenter

- created a getter for model._context so the property can be accessed using the getter
- remove pairing table constants
@yusufjimoh
Copy link
Contributor Author

Code has been updated to fix an issue on the pairing table pointed out by @adriazalvarez, Thanks.

adriazalvarez
adriazalvarez requested changes Nov 5, 2024
View reviewed changes
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 latest changes Yusuf.
I have been testing the pairing table again, everything works as expected.

adriazalvarez
adriazalvarez previously approved these changes Nov 5, 2024
View reviewed changes
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.

I forgot to approve before!

@cailafinn cailafinn self-assigned this Nov 6, 2024
cailafinn
cailafinn requested changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@cailafinn cailafinn left a comment

Choose a reason for hiding this comment

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

Just noticed a couple more improvements that could be made while we're editing these files.

...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated

def enable_updates(self):
"""Allow update signals to be sent."""
self._view.set_is_updating(False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this call seems to be backwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriazalvarez what do you think about this, do you think the function should be renamed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, probably the naming is confusing in this case, although I see how it is name as it is, as the function really means enable signals, and is_updating is a flag to not process view signals. Perhaps a clearer name could be unblock_update_signals and block_update_signals for disable_updates.
What do you think, @cailafinn ?

Copy link
Contributor

@cailafinn cailafinn Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the best way would be to rename the functions in the view to be disable_updates and enable_updates. is_updating and the term signals are implementation details of the view and Qt, so ideally should not be used as terms in the view's public-facing API.

...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...es/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_presenter.py Outdated Show resolved Hide resolved
...erfaces/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_view.py Show resolved Hide resolved
...erfaces/mantidqtinterfaces/Muon/GUI/Common/pairing_table_widget/pairing_table_widget_view.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriazalvarez adriazalvarez adriazalvarez left review comments

@cailafinn cailafinn cailafinn requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@cailafinn cailafinn

@adriazalvarez adriazalvarez

Labels
ISIS Team: Spectroscopy Issue and pull requests managed by the Spectroscopy subteam at ISIS Maintenance Unassigned issues to be addressed in the next maintenance period. Muon Issues and pull requests related to muons
Projects
DEVS
Status: Waiting response
Milestone
Release 6.12
Development

Successfully merging this pull request may close these issues.

Logic in the PairingTableView
3 participants
@yusufjimoh @cailafinn @adriazalvarez