Code review changes for new REFL gui

[Owen Arnold]

Nice work on 9371. Here are some changes i suggest from my code review.

1) Missing : TODO : DESCRIPTION in headers

2) No need for Code/Mantid/MantidQt/CustomInterfaces/src/IReflPresenter.cpp

3) Mark slots as such in the implementation documentation for each. Makes them easier to read and identify what they relate to.

4) Declare and initialise close to use in ReflLoadedMainViewPresenter::hasValidModel()

5) CreateTransmissionWorkspaceAuto should not need the following. Just delete the lines and test that it still works (creating a transmission workspace)

algCreateTrans->setProperty("Params",HACKYPARAMS); //HACK FOR NOW
algCreateTrans->setProperty("StartOverlap",10.0); //HACK FOR NOW
algCreateTrans->setProperty("EndOverlap",12.0); //HACK FOR NOW

7) Transmission workspaces should be called TRANS_{0} or TRANS_{0}_{1} where {0} and {1} should be substituted for run numbers of the transmission workspaces. See refl_gui.py

8) Multiple definitions of the same type : class MockView : public ReflMainView

Instead, create a MockObjects.h and put in the test directory. Include it where needed, but don't add to cmake test list.

9) Tests are hard to read (not your fault)

• Use TSM_ASSERT where possible, as this provides the ability to write a custom message explaining what the assertion is actually checking.
• Comment each derived class from ReflMainView in the tests. Explain what it is being used for.
• Evaluate the need for so many Fake types. Add ones that are really needed, and could be shared to the aforementioned MockObjects.h. Better to refactor the code in such a way that Fakes are not needed altogether.

10) Created Tranmsission workspaces (see 7) should not be deleted at the end of processing.

11) Add a bold Prototoype label to the GUI. I don't want users accidently picking this up at this stage.

Things to think about, but not necessarily correct:

Might be better to inject a new object rather than create one in an opaque fashion. For example QtReflMainView::setNew()

QtReflMainView::getFlag() has unsafe behaviour equivalent to pop found on many containers. Method has two responsibilities. What should happen if it fails to achieve one or the other of the responsibilities. What happens if another instance of getFlag is run from another thread at the same time? Is it better that the view clears the flags, or that the presenter commands it?

Algorithm execution should keep workspaces out of the ADS until required otherwise. Use setChild(true) on the algorithms. For example of misuse see ReflMainViewPresenter::processRow()

[Harry Jeffery]

Refs #10222 Provide class descriptions in headers.

Changeset: d19179b887d67c283c2d586fcfbe8f3651aa948a

[Harry Jeffery]

Refs #10222 Add prototype label to new refl gui

Changeset: 4480056c22e866eb21217cc62f2e13ff6d62c991

[Harry Jeffery]

Refs #10222 Remove IReflPresenter.cpp

Changeset: fae3e7e19f6c1a94c5db5aaa5e01c31f55fdcce6

[Harry Jeffery]

Refs #10222 Refactor exception usage in hasValidModel()

Changeset: 8b400aec70571622e76d07dacf3b2776f2395df4

[Harry Jeffery]

Refs #10222 Remove uneeded CreateTransmissionWorkspaceAuto params

Changeset: 101d1936d56de52d3b24c757c4e1481cadc5fbad

[Harry Jeffery]

Refs #10222 Fix transmission workspace validation

Input transmission workspaces were incorrectly failing validation when loaded with a child algorithm. This was because they were never entered into the ADS, so they were being wrongly seen as default input values.

Changeset: 48b1588e786b487ffd82c30a7baa8621edd53bfa

[Harry Jeffery]

Refs #10222 Rework processRow

• Run algorithms as children and only put workspaces in ADS at the last possible moment

• Cache transmission workspaces more effectively

• Expose TOF and transmission workspaces to the user at the end of processing.

Changeset: befdb4e7b2e3684182851e283c4825a2076a2a88

[Harry Jeffery]

Refs #10222 Remove NullMainViewPresenterTest

Changeset: 95d813700467a6aede53a7d648aa9222a3115846

[Harry Jeffery]

Refs #10222 De-duplicate RefMainView mock objects

Changeset: 1aad07cec058376a2b83f676213023b99c2b47b1

[Harry Jeffery]

Refs #10222 Document QtReflMainView slots more explicitly

Changeset: 6f70a77463f86a3aa089dd8c82b5f981591fec30

[Harry Jeffery]

Refs #10222 Merge getUserString into askUserString

Changeset: 7cb12998154922ab59c7a49c836e49989baa9301

[Harry Jeffery]

Refs #10222 Refactor IReflPresenter's notify method

We no longer pass flags separately. By passing them one at a time directly with notify we bypass a potential race condition and vastly simplify unit testing logic.

Changeset: cdcbbc9174cdd77d5d4ddd869beacc428d9dc25c

[Harry Jeffery]

Refs #10222 Fix ReflGui unit tests

Changeset: 8d0ab7abec205975c564ebac1a57e21784db7a6a

[Harry Jeffery]

Refs #10222 Fix broken unit test

Changeset: fffbc6b288aab745696b73ac08351c7d84764585

[Owen Arnold]

Merge remote-tracking branch 'origin/feature/10222_refl_gui_review_changes'

Full changeset: ​a5a0bfd32bcb598f974940f2dd760b8245b1616d

[Stuart Campbell]

This ticket has been transferred to github issue ​11064