Ticket #6149 (closed: fixed)

Opened 8 years ago

Last modified 5 years ago

Merge RefDetectorViewer code back into ImageViewer package

Reported by: Russell Taylor Owned by: Russell Taylor
Priority: critical Milestone: Release 2.4
Component: MantidPlot Keywords:
Cc: mikkelsond@…, bilheuxjm@… Blocked By:
Blocking: Tester: Jean Bilheux

Description

As previously discussed, it is a maintenance issue going forward if we have largely duplicated code existing in two separate places. Therefore we want to recombine these two code-bases to the extent that it is possible/sensible.

I see three main parts:

  • Remove classes under RefDetectorViewer that have not been modified at all and point back to the ImageViewer equivalent (checking that the latter has not changed in the interim in a problematic way).
  • For those classes that have only been changed in a minor way, try to modify the ImageViewer equivalent by, e.g., adding method arguments, method overloads or extra methods.
  • For those that have changed significantly, or are entirely new classes, consider whether to keep separate classes (with names that reflect their usage) or introduce subclasses.

I anticipate that in the end the separate RefDetectorViewer package will disappear, but there will be a separate UI class within the ImageViewer folder that will be the entry point for reflectometers.

Attachments

Ref-ImageView-diffs.txt (108.3 KB) - added by Russell Taylor 8 years ago.

Change History

comment:1 Changed 8 years ago by Russell Taylor

  • Owner set to Russell Taylor
  • Status changed from new to assigned

I will deal with the 'low-hanging fruit' (i.e. point 1, maybe parts of point 2).

comment:2 Changed 8 years ago by Russell Taylor

  • Status changed from assigned to accepted

comment:3 Changed 8 years ago by Russell Taylor

The following classes have not changed in any way and so can easily be removed from RefDetectorViewer:

  • Colomaps
  • DataArray (one minor irrelevant change that can be dropped)
  • ErrorHandler
  • IVUtils
  • ImageViewNxEventFile.cpp (test executable not used on the Ref side)
  • QtUtils
  • TrackingPicker
  • DllOption.h (header only)

The following classes have trivial changes so can also be removed:

  • RefImageDataSource - only the class name has been changed
  • ArrayDataSource - no change except the name of the base class (RefImageDataSource)
  • RefArrayDataSource - a renamed copy was introduced but the original left around

The following classes are at the top level and will almost certainly remain distinct (I didn't examine them for differences:

  • RefImageView
  • RefMatrixWSImageView
  • RefIVConnections (could be considered for commonality, but not right now)
  • RefDetectorViewDemo

This leaves 6 classes for which there are some differences:

  • MatrixWSDataSource - Unchanged on the Ref side. An 'emode handler' has been added on the ImageViewer side, but the behaviour should be unchanged if this is not set.
  • GraphDisplay - References to 'graph_table' commented out on the Ref side. In the ImageViewer class, we should add the possibility of this not appearing if so signalled at construction.
  • ImagePlotItem - There's some additional code in at the end of the draw() method. Put this code in a subclass and call the base class method from it? This class is referenced in (Ref)ImageDisplay and nowhere else.
  • ImageDisplay - Some methods (ShowPeakBackSelectionValue & related) and supporting data members have been added. The ShowPeakBackSelectionValue() is called in the existing SetPointedAtPoint() method and an argument has been added to the method. The other new methods are called from RefImagePlotItem (circular dependency?). Consider subclassing.
  • SliderHandler - No changes, but holds a reference to the Ui_(Ref)ImageViewer class which is a different class in the two systems.
  • RangeHandler - As above, holds a reference to the Ui_(Ref)ImageViewer class. Code referring to a 'y' direction (in addition to 'x') has been added and a few 'step' bits commented out on the Ref side. Although we could consider some kind of polymorphism (here and/or in the ImageViewer class) it is felt that this may be an impediment to future changes so the two handler classes will likely remain separate.

A diff showing the changes in the above classes as of today is attached to this ticket.

Changed 8 years ago by Russell Taylor

comment:4 Changed 8 years ago by Russell Taylor

Re #6149. Remove entirely unused file.

Changeset: f561aee13c62f1e832074811a306844a189ac05b

comment:5 Changed 8 years ago by Russell Taylor

Re #6149. Remove renamed ArrayDataSource class.

It is otherwise unchanged from the original. As the original was left around by mistake, just use that for now.

Changeset: 1b39e6508a75314bc2a3d4463b53ac430872f2c2

comment:6 Changed 8 years ago by Russell Taylor

Re #6149. Remove classes that are unchanged w.r.t. ImageViewer.

Will not compile right now - namespace issues need resolving.

Changeset: d5819afa2acb639a406fd59751896e749787fc3f

comment:7 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

This generally won't be necessary once everything's merged back into the same package (& namespace) but it gets things compiling again right now.

Changeset: 0c6e87fe78c043d81c5badbcc9ae7665e97b7def

comment:8 Changed 8 years ago by Russell Taylor

Re #6149. Extend include path so that python module will compile.

Changeset: b48905ce271169194ff440ced0bd548bf58cad69

comment:9 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefImageDataSource class.

It was identical to ImageView::ImageDataSource apart from the name change. Again, namespace qualifications are needed to allow things to compile.

Changeset: 699a8c32f85b2811425d41e6d3bf91ab204f7d82

comment:10 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

Changeset: 7f411501286d9937e12c3024af808230435464a3

comment:11 Changed 8 years ago by Russell Taylor

Re #6149. Remove ArrayDataSource class, which hadn't been changed.

Again, namespace fixed to follow.

Changeset: affb1138e5cb38503815f6accfbee03a5a483bd2

comment:12 Changed 8 years ago by Russell Taylor

Re #6149. Namespace qualification where necessary.

Changeset: bb4be8a0b58bac7430d5c93e4e99a86937327b62

comment:13 Changed 8 years ago by Russell Taylor

Re #6149. Allow for the absence of the GraphDisplay table.

The RefDetectorViewer doesn't show the table that the ImageViewer has alongside the line graphs. The GraphDisplay table holds a pointer to this object in order to populate it. Allow for this to be NULL by skipping the relevant code in this case.

Changeset: 8482e686de23252b965a451d03c9377612ab3c3e

comment:14 Changed 8 years ago by Russell Taylor

Re #6149. Remove GraphDisplay and point at ImageViewer version.

Changeset: ba09eccd3feee525c81d462cd33031a3fb2492a4

comment:15 Changed 8 years ago by Russell Taylor

Re MatrixWSDataSource - a hiccup with merging this one is that the ImageView version holds a reference to the EModeHandler, which in turn refers to the Ui_ImageViewer. This could probably be overcome via inheritance, but for now I'm going to skip over it.

comment:16 Changed 8 years ago by Russell Taylor

Re #6149. Rename RangeHandler & SliderHandler on the 'Ref' side.

They can't be merged because they reference the ui class. The rename is for clarity as other things are merged.

Changeset: ff1dd99d851cac23499427195aa8b83e606d14e6

comment:17 Changed 8 years ago by Russell Taylor

Re #6149. Minor changes to ImageViewer code to help Ref integration.

Main change is the introduction of intefaces for the Range & Slider handlers in order to decouple the link to the ui class. Also a few things moved from private to protected for the benefit of RefDetectorViewer derived classes (coming soon).

Changeset: 0cd592fd6254e122eef31c58c9c1ead139aceddd

comment:18 Changed 8 years ago by Russell Taylor

Re #6149. Major changes to RefDetectorViewer to eliminate duplication

...of code w.r.t. the ImageViewer.

The small extensions in RefImageDisplay & RefImagePlotItem compared to their ImageViewer counterparts have been isolated and these classes now derive from the ImageViewer ones.

In order to disentangle some things a new class 'RefLimitsHandler' has been introduced to handle the getting and setting of the values in the top-right line edits (Peak/Back/TOF Left/Right/min/max). This is quite a lot simpler than the previous way as the values are only held by the QLineEdit objects and not separately in addition.

Changeset: 574116664a72f0b563f96f97d5587a0b193dd0da

comment:19 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefIVConnections::peak_back_tof_range_update methods.

They had remained alive from a previous implementation, but weren't doing anything now.

Changeset: 79dac1da816ed3fa67b85c1af65d8a0957776cea

comment:20 Changed 8 years ago by Russell Taylor

Re #6149. Make demo executables depend on related library.

I.e. ImageViewerDemo depends on and uses ImageViewer shard library instead of building all the source files again itself. Same for RefDetectorViewDemo.

Changeset: 5bb7b79930819d7e509c2590f48ae67630ad7d09

comment:21 Changed 8 years ago by Russell Taylor

Re #6149. Remove unused RefMatrixWSDataSource class.

It hadn't been changed since forking from ImageView::MatrixWSDataSource. If it is needed in the future, the ImageView class should be used following changes to add an interface to the EModeHandler (as done for RangeHandler & SliderHandler).

Changeset: f7cb5fc0fa87b20396fe32cfef213af92e0ad6b7

comment:22 Changed 8 years ago by Russell Taylor

Re #6149. Remove entirely unused file.

Changeset: 5230583de7bb417dd4878b24790f1455829e57dc

comment:23 Changed 8 years ago by Russell Taylor

Re #6149. Remove renamed ArrayDataSource class.

It is otherwise unchanged from the original. As the original was left around by mistake, just use that for now.

Changeset: 60f1938e66745eef7a6a1ec159a28ca41795525d

comment:24 Changed 8 years ago by Russell Taylor

Re #6149. Remove classes that are unchanged w.r.t. ImageViewer.

Will not compile right now - namespace issues need resolving.

Changeset: caa047ee4dd5c1b9de6296620f46f783ddd171bb

comment:25 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

This generally won't be necessary once everything's merged back into the same package (& namespace) but it gets things compiling again right now.

Changeset: 9c91ad1f74a67a85a7afa5318ef8ccf31918469b

comment:26 Changed 8 years ago by Russell Taylor

Re #6149. Extend include path so that python module will compile.

Changeset: 168aec5a1f478180adbfa70856115cefc300560f

comment:27 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefImageDataSource class.

It was identical to ImageView::ImageDataSource apart from the name change. Again, namespace qualifications are needed to allow things to compile.

Changeset: c6fa27a6aadc5deebc72bcb1737fb08d0e1f85d8

comment:28 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

Changeset: 340d69b696a06f2f2057e00d603f0cc212eea29c

comment:29 Changed 8 years ago by Russell Taylor

Re #6149. Remove ArrayDataSource class, which hadn't been changed.

Again, namespace fixed to follow.

Changeset: 32abf042b7fb73e0302a0ae9b5596c1d8e62fa51

comment:30 Changed 8 years ago by Russell Taylor

Re #6149. Namespace qualification where necessary.

Changeset: 8524f58e5d64fcfb0fe52f40cb5716fdb0af8d87

comment:31 Changed 8 years ago by Russell Taylor

Re #6149. Allow for the absence of the GraphDisplay table.

The RefDetectorViewer doesn't show the table that the ImageViewer has alongside the line graphs. The GraphDisplay table holds a pointer to this object in order to populate it. Allow for this to be NULL by skipping the relevant code in this case.

Changeset: cb6ad231efc96c2bbf5c478cb38369a98d5a9f4e

comment:32 Changed 8 years ago by Russell Taylor

Re #6149. Remove GraphDisplay and point at ImageViewer version.

Changeset: 025bca99a77e2f98e2acde62b9c4ec4e56e3eaa0

comment:33 Changed 8 years ago by Russell Taylor

Re #6149. Rename RangeHandler & SliderHandler on the 'Ref' side.

They can't be merged because they reference the ui class. The rename is for clarity as other things are merged.

Changeset: 820890491d44c5259f76cce655fa2b4fb183dc27

comment:34 Changed 8 years ago by Russell Taylor

Re #6149. Minor changes to ImageViewer code to help Ref integration.

Main change is the introduction of intefaces for the Range & Slider handlers in order to decouple the link to the ui class. Also a few things moved from private to protected for the benefit of RefDetectorViewer derived classes (coming soon).

Changeset: b1cbb20890d22703639de01075d692d601e6eaa7

comment:35 Changed 8 years ago by Russell Taylor

Re #6149. Major changes to RefDetectorViewer to eliminate duplication

...of code w.r.t. the ImageViewer.

The small extensions in RefImageDisplay & RefImagePlotItem compared to their ImageViewer counterparts have been isolated and these classes now derive from the ImageViewer ones.

In order to disentangle some things a new class 'RefLimitsHandler' has been introduced to handle the getting and setting of the values in the top-right line edits (Peak/Back/TOF Left/Right/min/max). This is quite a lot simpler than the previous way as the values are only held by the QLineEdit objects and not separately in addition.

Changeset: 4855edfa424a2d49a1b4495a42d99b77534ccc10

comment:36 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefIVConnections::peak_back_tof_range_update methods.

They had remained alive from a previous implementation, but weren't doing anything now.

Changeset: 88de8fcf5e24d3095489b925b726654904176e27

comment:37 Changed 8 years ago by Russell Taylor

Re #6149. Make demo executables depend on related library.

I.e. ImageViewerDemo depends on and uses ImageViewer shard library instead of building all the source files again itself. Same for RefDetectorViewDemo.

Changeset: 03630e840d755e810eae513880f7a6e29b5ab4f6

comment:38 Changed 8 years ago by Russell Taylor

Re #6149. Remove unused RefMatrixWSDataSource class.

It hadn't been changed since forking from ImageView::MatrixWSDataSource. If it is needed in the future, the ImageView class should be used following changes to add an interface to the EModeHandler (as done for RangeHandler & SliderHandler).

Changeset: 50baa1c1438af86568a203cfb658554333f5fa7e

comment:39 Changed 8 years ago by Russell Taylor

Re #6149. Windows fix.

Changeset: 04b2106b23a3edfeea182529d0282a145547fe62

comment:40 Changed 8 years ago by Russell Taylor

Re #6149. Fix compiler warnings.

Integer precision is used internally, so it should be used consistently from the start where possible.

Changeset: 8dd928adab5cee7008d0e7ed9b4379fcd80ddf95

comment:41 Changed 8 years ago by Russell Taylor

Re #6149. Include ImageViewer & RefDetectorViewer in doxygen build.

Changeset: 794fdf4d7a63a1249e6a80a7ff3061aa55b4a6c0

comment:42 Changed 8 years ago by Russell Taylor

Re #6149. Add a short README file.

Changeset: da2fd5545187c7c0cafddc2c191a55cbce51d052

comment:43 Changed 8 years ago by Russell Taylor

Re #6149. Remove entirely unused file.

Changeset: 1779998e01fe21e62cd8b5e1ba5437dda1e030cc

comment:44 Changed 8 years ago by Russell Taylor

Re #6149. Remove renamed ArrayDataSource class.

It is otherwise unchanged from the original. As the original was left around by mistake, just use that for now.

Changeset: a23c247e056c941a878cffd10847786643c94f1a

comment:45 Changed 8 years ago by Russell Taylor

Re #6149. Remove classes that are unchanged w.r.t. ImageViewer.

Will not compile right now - namespace issues need resolving.

Changeset: 68e4da46246d269d5c488d2a0d7887888984ce53

comment:46 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

This generally won't be necessary once everything's merged back into the same package (& namespace) but it gets things compiling again right now.

Changeset: dbd6ddf25297818465109ec7839959f6da3861da

comment:47 Changed 8 years ago by Russell Taylor

Re #6149. Extend include path so that python module will compile.

Changeset: d29132aab6fc56e9500b925ae5f3f8cc0543076a

comment:48 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefImageDataSource class.

It was identical to ImageView::ImageDataSource apart from the name change. Again, namespace qualifications are needed to allow things to compile.

Changeset: 0c08d96c0131dd5821d2adeb0e736af09dc3be45

comment:49 Changed 8 years ago by Russell Taylor

Re #6149. Namespace importing/qualification where necessary.

Changeset: b97c85607c34f8e0024ac88e3fdacc2009ea597a

comment:50 Changed 8 years ago by Russell Taylor

Re #6149. Remove ArrayDataSource class, which hadn't been changed.

Again, namespace fixed to follow.

Changeset: 89af7830a553698cffe13d717332b3c48b0c3698

comment:51 Changed 8 years ago by Russell Taylor

Re #6149. Namespace qualification where necessary.

Changeset: 54156d14271a0ee69b4713c618c06ba7aabdedf4

comment:52 Changed 8 years ago by Russell Taylor

Re #6149. Allow for the absence of the GraphDisplay table.

The RefDetectorViewer doesn't show the table that the ImageViewer has alongside the line graphs. The GraphDisplay table holds a pointer to this object in order to populate it. Allow for this to be NULL by skipping the relevant code in this case.

Changeset: 28bf34d5d844b784dcbebe6d75662b1475db77d8

comment:53 Changed 8 years ago by Russell Taylor

Re #6149. Remove GraphDisplay and point at ImageViewer version.

Changeset: 808235a4320cff7cf500e0dc055c0db703109098

comment:54 Changed 8 years ago by Russell Taylor

Re #6149. Rename RangeHandler & SliderHandler on the 'Ref' side.

They can't be merged because they reference the ui class. The rename is for clarity as other things are merged.

Changeset: 16a21e536aacc5dce50295c814de72c5c50d81f3

comment:55 Changed 8 years ago by Russell Taylor

Re #6149. Minor changes to ImageViewer code to help Ref integration.

Main change is the introduction of intefaces for the Range & Slider handlers in order to decouple the link to the ui class. Also a few things moved from private to protected for the benefit of RefDetectorViewer derived classes (coming soon).

Changeset: 6d7cee50d02eb44bf1a9bc51fe97adbe75e21f77

comment:56 Changed 8 years ago by Russell Taylor

Re #6149. Major changes to RefDetectorViewer to eliminate duplication

...of code w.r.t. the ImageViewer.

The small extensions in RefImageDisplay & RefImagePlotItem compared to their ImageViewer counterparts have been isolated and these classes now derive from the ImageViewer ones.

In order to disentangle some things a new class 'RefLimitsHandler' has been introduced to handle the getting and setting of the values in the top-right line edits (Peak/Back/TOF Left/Right/min/max). This is quite a lot simpler than the previous way as the values are only held by the QLineEdit objects and not separately in addition.

Changeset: a5186a3b97145264dabbaf90c037b78890bf6c3b

comment:57 Changed 8 years ago by Russell Taylor

Re #6149. Remove RefIVConnections::peak_back_tof_range_update methods.

They had remained alive from a previous implementation, but weren't doing anything now.

Changeset: 5fa4080a61003b6bf591fa059b0d6d8efc060af2

comment:58 Changed 8 years ago by Russell Taylor

Re #6149. Make demo executables depend on related library.

I.e. ImageViewerDemo depends on and uses ImageViewer shard library instead of building all the source files again itself. Same for RefDetectorViewDemo.

Changeset: 4970fe55919b94402bc16202299e1a5f9e80f364

comment:59 Changed 8 years ago by Russell Taylor

Re #6149. Remove unused RefMatrixWSDataSource class.

It hadn't been changed since forking from ImageView::MatrixWSDataSource. If it is needed in the future, the ImageView class should be used following changes to add an interface to the EModeHandler (as done for RangeHandler & SliderHandler).

Changeset: 71b09f7d314245ae8d9160724adac59a4d840ba2

comment:60 Changed 8 years ago by Russell Taylor

Re #6149. Windows fix.

Changeset: e9017c8488acb8d3ff9163ace5de73e31af345f0

comment:61 Changed 8 years ago by Russell Taylor

Re #6149. Fix compiler warnings.

Integer precision is used internally, so it should be used consistently from the start where possible.

Changeset: deaf4e10a10db0a1f0f2fbf5d903328446102f0d

comment:62 Changed 8 years ago by Russell Taylor

Re #6149. Include ImageViewer & RefDetectorViewer in doxygen build.

Changeset: df2feba03d2d0cd6c664433efc2558ad4881b0da

comment:63 Changed 8 years ago by Russell Taylor

Re #6149. Add a short README file.

Changeset: e71dfcf451d5d6371ba91e4fc3935a788a1e5a78

comment:64 Changed 8 years ago by Russell Taylor

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

All merged into master and no complaints received as far as I know.

The formal testing step on this one should probably be carried out by Jean or Dennis. One thing to note is that the new collaboration diagrams etc. are now included in the doxygen docs - see http://doxygen.mantidproject.org

comment:65 Changed 8 years ago by Jean Bilheux

  • Status changed from verify to verifying
  • Tester set to Jean Bilheux

comment:66 Changed 8 years ago by Jean Bilheux

  • Status changed from verifying to reopened
  • Resolution fixed deleted

Tested with Liquids and Magnetism reflectometers. The application itself is working but does not communicate anymore to the python interface to display live the peak, background and TOF selection.

comment:67 Changed 8 years ago by Russell Taylor

  • Status changed from reopened to accepted

comment:68 Changed 8 years ago by Russell Taylor

Revert "Re #6149. Remove RefIVConnections::peak_back_tof_range_update methods."

This reverts commit 88de8fcf5e24d3095489b925b726654904176e27.

Changeset: 7ad5f71bdf14211b1f0ca7de000299169e344a82

comment:69 Changed 8 years ago by Russell Taylor

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

comment:70 Changed 8 years ago by Jean Bilheux

  • Status changed from verify to verifying

comment:71 Changed 8 years ago by Jean Bilheux

  • Status changed from verifying to closed

Ran the application from the liquids and from the Magnetism reflectometers and it works perfectly now for both of them. This ticket can be closed.

comment:72 Changed 8 years ago by Russell Taylor

Revert "Re #6149. Remove RefIVConnections::peak_back_tof_range_update methods."

This reverts commit 88de8fcf5e24d3095489b925b726654904176e27.

Changeset: 7ad5f71bdf14211b1f0ca7de000299169e344a82

comment:73 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 6995

Note: See TracTickets for help on using tickets.