Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Common data table model #1959

Draft
wants to merge 42 commits into
base: develop
Choose a base branch
from

Conversation

trisyoungs
Copy link
Member

@trisyoungs trisyoungs commented Jul 10, 2024

Implements a standard interface between QAbstractTableModel and any of our custom classes requiring such a display in the UI.

TODO

  • Implement insertRows()
  • Implement removeRows()
  • Flags for items
  • Icons for items
  • Test notification of Qt models when change occurs in underlying data programmatically
  • ExpressionVariableVectorKeyword needs to update related nodes when a parameter is removed. or added, or renamed.
  • Filtering Standard Qt proxy model when required
  • Need to notify main GUI when data changed

Conversions

Tables with Filter Proxies

  • AtomTypeModel
  • BraggReflectionModel
  • SitesModel
  • SpeciesSiteModel

Tables

  • DataManagerSimulationModel
  • MasterTermModel (inherited)
  • ModuleModel
  • RangeVectorModel
  • SpeciesAtomModel
  • SpeciesBondModel
  • SpeciesAngleModel
  • SpeciesTorsionModel
  • SpeciesImproperModel
  • WeightedModuleModel
  • XMLBondModel
  • XMLAngleModel
  • XMLTorsionModel
  • XMLImproperModel

QAbstractListModels

  • ConfigurationModel
  • EnumOptionsModel
  • ExternalPotentialModel
  • SpeciesModel

Closes #1902.

@trisyoungs
Copy link
Member Author

@rprospero As mentioned earlier some quite significant evolution has happened here. There's a lot going on, but the summary is captured below. Thoughts, comments, and ideas for alternative approaches very welcome.

  • I have namespaced everything within DataModel - this makes things quite neat, but is slightly against how the rest of our code is structured.
  • A class which is to be modeled now derives from DataModel::Modelable - this simple template class defines some useful usings for subsequent definition of setter and getter functions, and a single static function modelableProperties() which is called by the model itself to determine what properties are available.
  • Rather than monolithic setter and getter functions acting on all available properties I elected to move to per-property definition. This removes the need for any enumeration of the type and simplifies things quite a bit. The necessary data setters and getters are defined locally in the class source file by implementing DataModel::Modelable<T>::modelableProperties() , next to other related functions.
  • I have moved towards a VectorModelable class which owns the std::vector it is acting as a model for, rather than taking a reference to an object elsewhere. The simple reason for this is to ensure that we can track programmatic mutations of the underlying std::vector data, at least to a level that enables the GUI to reflect the changes automatically without impinging too much on us actually writing clear code.
  • The template for VectorModelable currently takes two classes - the type of the underlying data (e.g. ExpressionVariable) and its storage type in the vector (e.g. std::shared_ptr<ExpressionVariable>). This is necessary to achieve transparent access via the getters and setters (which just operate on a pointer to an object of the underlying data since the actual mechanism of storage is irrelevant). This isn't terribly neat, and I'm sure there's a clever C++-23 way of achieving the same with an "auto-deduced template mangle", but I'm not clever enough to figure out how.
  • Since we will have both plain and pointer types stored in our std::vector, the creation function must be manually defined always and cannot be assumed, especially since we may be storing pointers to base class types that are pure virtual on their own.
  • Const-access to the underlying vector data is always available, but when programmatic editing of the underlying vector is required we have a function DataModel::Table::mutableData() which returns a VectorMutator object alongside the non-const vector. The purpose of VectorMutator is simply to signal beginResetModel() on construction and endResetModel() when it goes out of scope. This is the probably going to be the biggest intrusion on the codebase, but feels like a small price to pay for a general model which allows the GUI to react to programmatic changes automatically.

Copy link
Contributor

@rprospero rprospero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like this setup and it feels clean. I've made some comments about some places where I might have handled the types differently, but there may be corner cases that I haven't considered.

src/templates/dataModelItem.h Outdated Show resolved Hide resolved
src/gui/dataModelTableInterface.cpp Outdated Show resolved Hide resolved
src/templates/dataModelItem.h Outdated Show resolved Hide resolved
src/templates/dataModelItem.h Outdated Show resolved Hide resolved
src/templates/dataModelItem.h Show resolved Hide resolved
src/templates/dataModelBase.h Outdated Show resolved Hide resolved
src/templates/dataModelVectorModelable.h Outdated Show resolved Hide resolved
src/templates/dataModelVectorModelable.h Show resolved Hide resolved
src/templates/dataModelVectorModelable.h Outdated Show resolved Hide resolved
@trisyoungs trisyoungs added this to the Release 2.0 milestone Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Model Interface for Qt
2 participants