Ticket #10431 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

ReflMainViewPresenter goes behind the model's back

Reported by: Harry Jeffery Owned by: Harry Jeffery
Priority: major Milestone: Release 3.3
Component: Reflectometry Keywords:
Cc: Blocked By:
Blocking: #10440 Tester: Owen Arnold

Description

Currently the presenter does not use the ReflTableModel to manipulate the table, it goes through the table workspace itself. This stops the model from receiving proper updates on what has changed, and therefore from updating the view correctly. To get around this currently, every time the presenter changes the table it is reloaded into the view. This means that selections and table settings cannot be preserved.

Instead of passing the ReflMainView a table workspace pointer, ReflMainViewPresenter should pass it a ReflTableModel which it shall use itself, allowing the table to be correctly updated.

Change History

comment:1 Changed 6 years ago by Harry Jeffery

Refs #10431 Extend QReflTableModel

  • Add insertRows
  • Add deleteRows

Changeset: 6f5a68928ca55f2cf5f786324006d325bd60e07a

comment:2 Changed 6 years ago by Harry Jeffery

Refs #10431 Create default table in helper

Changeset: bef0927e6d57f751dc4d820375cdeb9034f771da

comment:3 Changed 6 years ago by Harry Jeffery

Refs #10431 Replace size_t with int

Qt uses int instead of size_t for row indices. Change our usage to match Qt's to prepare for using QReflTableModel.

Changeset: 66971df5187aff6afdc3b3d652c27c106e4e58c5

comment:4 Changed 6 years ago by Harry Jeffery

Refs #10431 Pass QReflTableModel to QtReflMainView

The view will now automatically stay in sync with the model, because we're not going behind the model's back in the presenter.

Changeset: 14d9842b42c886df1d1f2d711e5bde93017bae79

comment:5 Changed 6 years ago by Harry Jeffery

  • Status changed from new to assigned

comment:6 Changed 6 years ago by Harry Jeffery

  • Status changed from assigned to inprogress

comment:7 Changed 6 years ago by Harry Jeffery

Refs #10431 Resynchronise documentation

Changeset: 94444493c680411e8259072103c291c549a64440

comment:8 Changed 6 years ago by Harry Jeffery

Refs #10431 Update unit tests

Changeset: 4e5752a0c0d85e6a42e7c3ccad5dd0ecec113683

comment:9 Changed 6 years ago by Harry Jeffery

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewPresenterTest.h

Changeset: 1434c96f42676b13ee7f44d50671fa5fc7e088b7

comment:10 Changed 6 years ago by Harry Jeffery

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

Testing

The changes introduced by this ticket are subtle, but big wins for usability. When performing actions on the model the users' selection will now be preserved and moved as appropriate. This is very useful when inserting rows in-place, or grouping and then processing.

  • Verify unit tests are passing
  • Inspect changes
  • Play with row manipulation (especially with the context menu)
    • Your selection should be preserved for all actions (deleting will of course clear your selection)

Branch: https://github.com/mantidproject/mantid/compare/feature/10431_refl_presenter_use_model

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

comment:11 Changed 6 years ago by Harry Jeffery

  • Blocking 10440 added

comment:12 Changed 6 years ago by Owen Arnold

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

comment:13 Changed 6 years ago by Owen Arnold

I agree with these changes, but do not agree with specifying high levels of precision as this is going to be brittle. https://github.com/mantidproject/mantid/blob/1434c96f42676b13ee7f44d50671fa5fc7e088b7/Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewPresenterTest.h#L713 use http://cxxtest.com/guide.html#ts_assert_delta instead where this kind of thing has been put in place.

comment:14 Changed 6 years ago by Owen Arnold

  • Status changed from verifying to reopened
  • Resolution fixed deleted

comment:15 Changed 6 years ago by Harry Jeffery

  • Status changed from reopened to inprogress

Refs #10431 Allow some variance in expected DQQ

Changeset: bcd6011051db70aa1a2838b9988a011f91b614af

comment:16 Changed 6 years ago by Harry Jeffery

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

comment:17 Changed 6 years ago by Harry Jeffery

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/inc/MantidQtCustomInterfaces/ReflMainView.h Code/Mantid/MantidQt/CustomInterfaces/src/ReflMainViewPresenter.cpp Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewMockObjects.h

Changeset: ebd4ecb968330cbcb34c539c7e71b8a0d5bf5506

comment:18 Changed 6 years ago by Harry Jeffery

  • Blocking 10457 added

comment:19 Changed 6 years ago by Harry Jeffery

  • Blocking 10457 removed

comment:20 Changed 6 years ago by Owen Arnold

  • Status changed from verify to reopened
  • Resolution fixed deleted

Needs merge conflicts resolved.

comment:21 Changed 6 years ago by Harry Jeffery

  • Status changed from reopened to inprogress

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewMockObjects.h

Changeset: 69a06db56ced9f80876bb904731f016371bc5ec3

comment:22 Changed 6 years ago by Harry Jeffery

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

comment:23 Changed 6 years ago by Harry Jeffery

  • Status changed from verify to closed

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewPresenterTest.h

Full changeset: 1434c96f42676b13ee7f44d50671fa5fc7e088b7

comment:24 Changed 6 years ago by Harry Jeffery

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/inc/MantidQtCustomInterfaces/ReflMainView.h Code/Mantid/MantidQt/CustomInterfaces/src/ReflMainViewPresenter.cpp Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewMockObjects.h

Full changeset: ebd4ecb968330cbcb34c539c7e71b8a0d5bf5506

comment:25 Changed 6 years ago by Harry Jeffery

Merge branch 'master' into feature/10431_refl_presenter_use_model

Refs #10431

Conflicts:

Code/Mantid/MantidQt/CustomInterfaces/test/ReflMainViewMockObjects.h

Full changeset: 69a06db56ced9f80876bb904731f016371bc5ec3

comment:26 Changed 6 years ago by Owen Arnold

Merge remote-tracking branch 'origin/feature/10431_refl_presenter_use_model'

Full changeset: d48fb15186e08856c0a60b4ac6a543c06da1e211

comment:27 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 11273

Note: See TracTickets for help on using tickets.