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: Master term models #1893

Merged
merged 5 commits into from
May 29, 2024
Merged

Conversation

trisyoungs
Copy link
Member

This PR moves the responsibility of adding / removing master terms to the relevant model, rather than leaving this the job of the GUI at the calling point.

A significant change here is the storage of a reference to CoreData within the base MasterTermModel. Comments welcome on this, but the allows easy access to the addition / removal of terms, as well as permitting a nice clean up of code throughout all the models.

@rprospero
Copy link
Contributor

Using the CoreData as the parameter on the constructor certainly simplifies things. The one bit that makes me nervous is that requiring a parameter means that the type cannot be created in QML. Creating the type in the QML itself can be useful (e.g. what we do with AddForcefieldDialogModel in AddForcefieldTermsDialog.qml). However, we're not doing that anywhere with the master terms and it doesn't seem likely that we will, so we should be okay.

I'm bringing this up because I've been thinking about the structure of the models for the pure QML GUI. I'm imagining that we'll ultimately need a bunch of higher level models (e.g. DissolveModel, CoreDataModel) and that these might be able to return inner models (e.g. a CoreDataModel having a function that returns its MasterAngleModel). That way the models can be passed around as parameters within the QML and can be passed into any other bespoke models that may need them.

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.

There's a bunch of places where we might want to add some error logging and/or dialogs, but it looks good otherwise.

src/gui/forcefieldTab.cpp Outdated Show resolved Hide resolved
src/gui/forcefieldTab.cpp Outdated Show resolved Hide resolved
src/gui/forcefieldTab.cpp Outdated Show resolved Hide resolved
src/gui/forcefieldTab.cpp Show resolved Hide resolved
src/gui/forcefieldTab.cpp Show resolved Hide resolved
@trisyoungs
Copy link
Member Author

Using the CoreData as the parameter on the constructor certainly simplifies things. The one bit that makes me nervous is that requiring a parameter means that the type cannot be created in QML. Creating the type in the QML itself can be useful (e.g. what we do with AddForcefieldDialogModel in AddForcefieldTermsDialog.qml). However, we're not doing that anywhere with the master terms and it doesn't seem likely that we will, so we should be okay.

I'm bringing this up because I've been thinking about the structure of the models for the pure QML GUI. I'm imagining that we'll ultimately need a bunch of higher level models (e.g. DissolveModel, CoreDataModel) and that these might be able to return inner models (e.g. a CoreDataModel having a function that returns its MasterAngleModel). That way the models can be passed around as parameters within the QML and can be passed into any other bespoke models that may need them.

Yes, this is a very valid concern, and not one which I had thought about when I made that change! However, your suggestion of a top level model encompassing either or both of Dissolve and CoreData matches closely with what @jws-1 had started tinkering around with for "Workflow 2.0", so I think we are all of the same mind going forward.

@RobBuchananCompPhys RobBuchananCompPhys marked this pull request as ready for review May 20, 2024 09:09
@RobBuchananCompPhys RobBuchananCompPhys merged commit 1e9e0ed into develop May 29, 2024
10 checks passed
@RobBuchananCompPhys RobBuchananCompPhys deleted the refactor-master-models branch May 29, 2024 08:40
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.

None yet

3 participants