Ticket #10364 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

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:1 Changed 6 years ago by Owen Arnold

I agree on both counts with the following provisos/suggestions:

  • If the behaviour of the two presenters starts to deviate significantly, we need to encourage the creation of a new subclass rather than peppering the code with conditional statements.
  • I haven't thoroughly thought this through, but one way of keeping the same code, but ensuring type safety would be to use compile type polymorphism (i.e. templated code) see CRTP http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
  • Yes, the presenter should be making these decisions. I doubt that the presenter construction is expensive in terms of either memory or CPU time, but I agree, it is excessive. One thing that is also a problem with the View creating the presenter in the current scenario is that it needs to know which type of presenter to create, this is an violation of our 'stupid view' approach. One way to solve this would be to pass a FactoryMethod object to the view, but I digress, and this is an over-engineering of the current problem.

comment:2 Changed 6 years ago by Harry Jeffery

  • Status changed from new to assigned

comment:3 Changed 6 years ago by Harry Jeffery

  • Status changed from assigned to inprogress

comment:4 Changed 6 years ago by Harry Jeffery

  • Blocking 10304 added

(In #10304) Blocked by #10364, which is merging the two sets of unit tests.

comment:5 Changed 6 years ago by Harry Jeffery

This commit has been deleted.

Last edited 6 years ago by Harry Jeffery (previous) (diff)

comment:6 Changed 6 years ago by Harry Jeffery

This commit has been deleted.

Last edited 6 years ago by Harry Jeffery (previous) (diff)

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:8 Changed 6 years ago by Harry Jeffery

  • Blocking 10372 added

comment:9 Changed 6 years ago by Harry Jeffery

  • Blocking 10349 added

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:11 Changed 6 years ago by Harry Jeffery

  • Blocking 10376 added

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

(In #10380) Waiting for #10364 to pass before unit test can be 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

Note: See TracTickets for help on using tickets.