Ticket #9371 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

New MVP Reflectometry GUI - Base setup

Reported by: Keith Brown Owned by: Harry Jeffery
Priority: major Milestone: Release 3.3
Component: Reflectometry Keywords:
Cc: Blocked By: #9470, #9500, #9501
Blocking: #9368, #9865, #10222 Tester: Owen Arnold

Description (last modified by Owen Arnold) (diff)

This ticket will have the presenter, a presenter test class, and the abstract view class, as well as any other classes required to make this work to a basic level.

The presenter constructor should take a ITableworkspace and the view and call showTable() from the viewer passing the model as a parameter.

The end result should be able to:

  • Load and display a table workspace as a model.
  • Create a new table
  • Allow to process rows (and sets of rows) in the table via ReflectometryReductionOneAuto
  • Allow the user to save

Change History

comment:1 Changed 6 years ago by Keith Brown

  • Blocking 9373 added

comment:2 Changed 6 years ago by Nick Draper

  • Owner set to Keith Brown
  • Status changed from new to assigned

comment:3 Changed 6 years ago by Keith Brown

  • Status changed from assigned to inprogress

Refs #9371 View and Presenter basics and Test

The Presenter has been created which assignes a model to a view

The view base calss has been written. This is abstract as the actual view we'll be using will be Qt based

A test for the presenter has been written

Changeset: 6aa7936690caed680f0317d10794c8f16ad52eb8

comment:4 Changed 6 years ago by Keith Brown

Refs #9371 Presenter now subclassed from interface

The presenter is now a child of the new IReflPresenter, as is the new ReflNullMainViewPresenter

Changeset: 893dc433343eac691c508f7e0a6ece7e0cb3ede5

comment:5 Changed 6 years ago by Keith Brown

  • Description modified (diff)

There has been a slight change to the ticket's end goal. The gui has to be in a form that can load and display a table workspace as a model before this is deemed complete

The next step is as follows

I mentioned to Owen that the way we've got it now the interface would require a table workspace before it loads, when this interface should be able to create a blank and go from there.

As a result the presenter is being sub-classed from an interface and a Null object made for when there is no model. The constructor is also being overloaded to take a string rather than an already loaded table workspace in which case it checks the ADS and loads it if not in the ADS.

comment:6 Changed 6 years ago by Keith Brown

Refs #9371 Restructuring test slightly

A test needed moving inot a new class

Discovered i was missing system.h from includes as i needed it for the DLLExport

Changeset: 51c39a7f4a35e19ef06fca1bad2277e1df40c96e

comment:7 Changed 6 years ago by Keith Brown

Refs #9371 Updated Class Docs

The doxygen block in the header has been updated.

Changeset: 657e382e9e27deccd4df7cb9fb0f5c9761bd3fb3

comment:8 Changed 6 years ago by Owen Arnold

  • Description modified (diff)

comment:9 Changed 6 years ago by Keith Brown

Refs #9371 View and model are now presenter members

the presenter now keeps the view and model as member variables. Load() is used to put the model into the view.

Changeset: c307f078f6ef15d45d4beeaeef0ccdc0919d14e4

comment:10 Changed 6 years ago by Keith Brown

Refs #9371 Interface now loads tables into itself

The interface will now load and display any tableworkspaces in reflectometry format already in the ADS.

Changeset: a02651c25c15e926ef4b63729ee443625d54c55d

comment:11 Changed 6 years ago by Keith Brown

Refs #9371 Unit tests updated

The unit tests have been updated wiht the following:

The null object now tests notify() rather than load() as notify is all the view should do and load() is now private.

The concrete class now tests three fail cases where the table isn't in a valid format.

Changeset: 31e2c2fb8dd14bbdcfefa9cc4b54e6790f9cb3fd

comment:12 Changed 6 years ago by Keith Brown

  • Blocking 9373 removed

(In #9373) This functionality has been added to #9371 so this ticket is redundant

comment:13 Changed 6 years ago by Owen Arnold

Things to fix:

https://github.com/mantidproject/mantid/blob/31e2c2fb8dd14bbdcfefa9cc4b54e6790f9cb3fd/Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewPresenterTest.h#L61

  • Having a creational method like this ITableWorkspace_sptr createbadWorkspace(int typeflag = 1) that returns the product is good. But the naming isn't good. Use full camel case. Using lowercase with underscores is also acceptable.
  • The typeflag argument is completely opaque without looking at the body of the function. This is bad. Either provide good descriptive arguments for this creational method, or better yet, have separate functions for each variant that you need. For example create_bad_tableworkspace_too_many_columns(), create_bad_tableworkspace_too_few_columns() This will make the tests much more readable

comment:14 Changed 6 years ago by Keith Brown

Refs #9371 Split usage of the presenter into a loaded and blank.

ReflMainViewPresenter has been made abstract and sublassed to ReflBlankMainViewPresenter (which creates a blank workspace with one row on the fly) and ReflLoadedMainViewPresenter (which does exactly what ReflMainViewPresenter used to do)

The test for ReflMainViewPresenter, has been moved to ReflLoadedMainViewPresenter and has had the fixes owen mentioned applied.

Rejigged the layout on the UI, in leiu of a menubar, there's a temperary row of buttons that will act the same until i can move the functionaity elsewhere.

Changeset: 5a339702fd824fa05e2982693c347c3493393a86

comment:15 Changed 6 years ago by Keith Brown

Refs #9371 Editing and saving tables

Users can now edit and save changes to tables.

Changeset: 857709601cf15b8062970a92d999bec8b4c7e6e6

comment:16 Changed 6 years ago by Keith Brown

Refs #9371 Removed unused test

The test for the base ReflMainViewPresenter class was made redundant when it was subclassed.

Changeset: 17ff7e3f88d1efebab69cad90960947c4cafbea4

comment:17 Changed 6 years ago by Keith Brown

Refs #9371 Added buttons, flags and getters

Added "Add Row" and "Delete Row" buttons to interface

Added row headers to view as the code that would implement them was missing from the model

"Add Row", "Delete Row" and process have had appropriate flags, getters and connections made in the view

Changeset: 63b1bd911b0f451e1547a14ebcc319837c7a6cb0

comment:18 Changed 6 years ago by Keith Brown

Refs #9371 Add row functionality

Add Row will now append a row to the model. The new row will always be at the end for now.

Changeset: 1de63d7deac0d5f5ee367a316f7875cdd2ac7a65

comment:19 Changed 6 years ago by Owen Arnold

  • Blocked By 9470 added

comment:20 Changed 6 years ago by Keith Brown

Refs #9371 Add row, delete row and process

The add row functionality has been fleshed out to allow adding in speciifc places

The delete row functionality has been added and removes only when an entire row is selected

Processing is now possible, however it does contain a few hard-coded hacks to set values at present while owen fixes a fault in CreateTransmissionWorkspaceAuto

Methods to show differeing levels of message box have been added inluding Yes/No question, Critical, Warning and Information

The view can now return a vector<size_t> of selected columns.

Doxygen info has also been added

Changeset: a231f52e020fb1b0246c3fceb5c60afaa641af0d

comment:21 Changed 6 years ago by Keith Brown

Refs #9317 Add and delete row tests

Add row and delete row now have unit tests

Cleaned up a load of gmock warnings by creating a new class specifically for the constructor.

Changeset: 63193658fefb87a9386dd212bc1a7adf022b3ccf

Was marked against wrong ticket, copied here where it should be.

comment:22 Changed 6 years ago by Keith Brown

Refs #9371 Working on processing tests

Processing testing had been started, although this ticekt has now become blocked by the problem that the two Auto algorithms i use can't be accessed from a unit test, so that'll need fixed before i can continue.

Changeset: bb40c89d212ff717b54f260673c3b120fbc44a09

comment:23 Changed 6 years ago by Keith Brown

  • Blocked By 9500 added

comment:24 Changed 6 years ago by Keith Brown

  • Blocked By 9501 added

comment:25 Changed 6 years ago by Keith Brown

  • Owner changed from Keith Brown to Owen Arnold
  • Milestone changed from Release 3.2 to Backlog

not going to get this done before i leave, given to Owen

comment:26 Changed 6 years ago by Owen Arnold

  • Blocking 9865 added

comment:27 Changed 6 years ago by Owen Arnold

  • Owner changed from Owen Arnold to Harry Jeffery

comment:28 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix compilation

Changeset: 64812eb3c2625e38334918ae138dc555373be7ab

comment:29 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix initialiser order warning

Changeset: c9f6c2dc5e2114ce4c0a2c6d3c487d50edce468a

comment:30 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix private slots indentation

Changeset: e1531c228c3f3c0d399e3f6077e305c893985406

comment:31 Changed 6 years ago by Harry Jeffery

Refs #9371 Re-layout Reflectometry UI

The existing design was historical. This commit dispenses with some obsolete items and reorganises the existing items. The new layout is still a little bit rough, so there's room for improvement. But it's a step in the right direction.

Changeset: e75ded6f546a0b3fd35877ce6c4d947e91f4918b

comment:32 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix a few typos

Changeset: c8b90c5e1c5bb3c7f8084d0479a0d4c5d6bbd856

comment:33 Changed 6 years ago by Harry Jeffery

Refs #9371 Ensure no refl presenter notifications are ignored

Changeset: 0eb4b2145b4690f8e7eb706e1a19af4785625c9c

comment:34 Changed 6 years ago by Harry Jeffery

Refs #9371 Remove some magic numbers

Changeset: 61ffc2c8989a8caa26f7faa58320e74c0815600a

comment:35 Changed 6 years ago by Harry Jeffery

Refs #9371 Resolve a gcc warning

Changeset: abf8611ad74386705506c4d9f85e29657ab36eb3

comment:36 Changed 6 years ago by Harry Jeffery

Refs #9371 Clean up superfluous logic in deleteRow()

Changeset: 319f93e6bd3d14519355d7d55121a64b78cfe584

comment:37 Changed 6 years ago by Harry Jeffery

Refs #9371 Refactor makeTransWS

Changeset: 73929ffc63850ab60cebf74940617485de6e4896

comment:38 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix caching bug.

When there is only one row in the reflectometry gui, updating a value on it does not cause the table view to be updated. This was a caching related problem. This commit fixes that problem and optimises the dataChanged signal, so that only items that have been updated need to be fetched by the table view.

Changeset: 6e1e5ea96a32ddd3906f5cafeedcc008e3df1cb2

comment:39 Changed 6 years ago by Harry Jeffery

Refs #9371 Tidy up the Refl Gui a little more.

Changeset: fc7edfd56ee89bb382fb1e7fc3ae2f248eed26a7

comment:40 Changed 6 years ago by Harry Jeffery

Refs #9371 Refactor out some more magic numbers.

Changeset: 959648089395a51446a7e868fbd821a5f66bcbed

comment:41 Changed 6 years ago by Harry Jeffery

Refs #9371 Strip whitespace in Refl Gui table.

Changeset: ebd419ef1aecc92da1954db33563c0f774cd3dee

comment:42 Changed 6 years ago by Harry Jeffery

  • Milestone changed from Backlog to Release 3.3

comment:43 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix error when processing rows

Changeset: 5fff6522e73200713927e4b12b3094df683c7f2b

comment:44 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix warning in unit tests

Changeset: 74ef095808621d48c2e6c80c5f998ebdbc15354e

comment:45 Changed 6 years ago by Harry Jeffery

Refs #9371 Refactor ReflMainView flag interface

Before, each flag had its own boolean, and its own method to fetch that boolean. This was all very well, but then to add a new flag you'd have to add a new method in every child view, and then handle that method in every presenter that used the view. You'd also then have to fix every single unit test for the presenters. It was a bit brittle.

This commit replaces that with a single method getFlag which pops the first flag from an internal queue, a flag now being an enum. The presenter can check if there are any flags waiting to be processed by calling flagSet.

Presenters just fetch the flags until there are no more, and handle them as appropriate. Views just add the right flag to their internal queue when they like.

Changeset: c4ed068e6e0cbc99bd4d74df3f04cb004a15ac38

comment:46 Changed 6 years ago by Harry Jeffery

Refs #9371 Tidy ReflMainViewPresenter processing

Changeset: 53ada0700ac91d9060d9f64b93a6288373b6d6ad

comment:47 Changed 6 years ago by Harry Jeffery

Refs #9371 Make ReflMainView::askUserString more flexible

Changeset: e168b80a50a16cb02af10506ca64ce524ab66b53

comment:48 Changed 6 years ago by Harry Jeffery

Merge branch 'master' into feature/9371_Refl_MVP_Base_Setup

Refs #9371.

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/CMakeLists.txt

Changeset: 0b2e983f2a88c575ff48dd19b4ffd880d0ebbea6

comment:49 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix broken unit tests

I forgot to update the tests after changing the type signature of askUserString.

Changeset: 69e158149d19eb38ecc6ea4459d9bd43505cde9a

comment:50 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix Doxygen documentation warnings

Changeset: 529e453ac4c314eea86883dee8a7e42070caeb6b

comment:51 Changed 6 years ago by Harry Jeffery

Refs #9371 Fix doxygen documentation warnings 2

Changeset: 50075f0e8db77d50eeb3c2e8d2ba032685a7fdde

comment:52 Changed 6 years ago by Harry Jeffery

  • Status changed from inprogress to verify
  • Resolution set to fixed

comment:53 Changed 6 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:54 Changed 6 years ago by Owen Arnold

  • Blocking 10222 added

comment:55 Changed 6 years ago by Harry Jeffery

  • Status changed from verifying to closed

Merge branch 'master' into feature/9371_Refl_MVP_Base_Setup

Refs #9371.

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/CMakeLists.txt

Full changeset: 0b2e983f2a88c575ff48dd19b4ffd880d0ebbea6

comment:56 Changed 6 years ago by Owen Arnold

Merge remote-tracking branch 'origin/feature/9371_Refl_MVP_Base_Setup'

Full changeset: 2af01ab02ce817b3db414efb67d33a4d30fc7838

comment:57 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 10214

Note: See TracTickets for help on using tickets.