Ticket #10364 (closed: fixed)
Merge ReflBlankMainViewPresenter and ReflLoadedMainViewPresenter
Reported by: | Harry Jeffery | Owned by: | Harry Jeffery |
---|---|---|---|
Priority: | major | Milestone: | Release 3.3 |
Component: | Reflectometry | Keywords: | |
Cc: | Blocked By: | #10301, #10302 | |
Blocking: | #10304, #10349, #10372, #10376, #10380 | Tester: | Owen Arnold |
Description
Having interchangeable presenters can be very useful if their behaviour is radically different, such as a completely different analysis back end. However, these two presenters share 99% of their functionality through ReflMainViewPresenter.
The only difference in their behaviour is whether save() asks for a workspace name or not. This is trivial and should be handled as an if statement within a generic presenter, not a whole new presenter.
Additionally, when the user changes the workspace/model, the view destroys the presenter and creates an entirely new one to replace it. Rebuilding the entire presenter seems excessive and expensive. It also seems like it should be the presenter's prerogative when it is to be replaced. Instead of creating a new presenter, the view should notify the presenter that it wishes to load a different workspace and then the presenter can take care of the heavy lifting. If a new presenter is actually _required_ each time the workspace changes (which is doubtful), then the presenter can pass the view a new presenter to report to.
Another downside to having two separate presenters is we either have to duplicate a lot of testing, or just assume that success on one presenter implies success on the other.
Change History
comment:7 Changed 6 years ago by Harry Jeffery
- Blocked By 10302 added
#10302 is causing some nasty merge conflicts. Waiting for that to pass testing so that it can be merged into this branch.
comment:10 Changed 6 years ago by Harry Jeffery
- Blocked By 10301 added
Added blocked by #10301 to ensure I complete that work first, rather than perform it in the midst of this and end up with nasty merge conflicts.
comment:12 Changed 6 years ago by Harry Jeffery
Refs #10364 Merge Refl presenters into ReflMainViewPresenter
Changeset: 1dded72502f7701c04a21e45e257726ae1f60549
comment:13 Changed 6 years ago by Harry Jeffery
Refs #10364 Add unit tests for ReflMainViewPresenter
These unit tests are by no means complete, and are due for a complete overhaul in the near future. They do however cover almost all functionality, so they are sufficient for the moment.
Changeset: 53c51fc79b07f7b809ee7ef8911a446b2a3e1f56
comment:14 Changed 6 years ago by Harry Jeffery
Refs #10364 Remove ReflMainView.cpp
Changeset: 75675252bb72c830d8966c7973d74ab668b5a698
comment:15 Changed 6 years ago by Harry Jeffery
Refs #10364 Move instrument list logic out of ReflMainView
The presenter is now responsible for giving the view a list of valid instruments and selecting a default one.
Changeset: 0d4eca48ac9dc5846c5e3d8116fbbf528851c640
comment:16 Changed 6 years ago by Harry Jeffery
Refs #10364 Test loading invalid workspaces
The old tests were dropped during the merge. Let's re-add them.
Changeset: 051d75611562c5a909f9dacf1a2f014f0ef9664f
comment:17 Changed 6 years ago by Harry Jeffery
Refs #10364 Cheer up doxygen
Changeset: 10f9827938225da4e0d29bf179dd09c8c62977a0
comment:18 Changed 6 years ago by Harry Jeffery
Refs #10364 Fix build on Win7 and RHEL6
Changeset: 36052a719f31b78900c2725e74851cfa8b4f1e29
comment:19 Changed 6 years ago by Harry Jeffery
- Status changed from inprogress to verify
- Resolution set to fixed
Testing
- Inspect code
- Verify unit test is passing
- Play with the new Refl UI, verify nothing has broken
comment:20 Changed 6 years ago by Harry Jeffery
- Blocking 10380 added
comment:21 Changed 6 years ago by Owen Arnold
- Status changed from verify to verifying
- Tester set to Owen Arnold
comment:22 Changed 6 years ago by Owen Arnold
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/10364_merge_refl_presenters'
Full changeset: 6123bd47daefc28ddf88d7e9a8851f620cf5b8cc
comment:23 Changed 6 years ago by Dan Nixon
Add testing for Vesuvio resolution algorithm
Refs #10364
Changeset: dff3da88d77fbd339a34c95eac334e7d0a10989b
comment:24 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 11206
I agree on both counts with the following provisos/suggestions: