display qml scale charges dialog #1763
Conversation
RobBuchananCompPhys
force-pushed
the
scale-charges-dialog-qml-translation
branch
from
997efcd
to
a0011bf
Compare
There was a problem hiding this comment.
Okay, I've made a bunch of comments, so I want to take a moment here to say that I really like what you've done here. This is a great use of the model setup and the QML code is pretty clean.
Besides the individual comments that I've made (and which I'm more than willing to listen, if you disagree), there's one larger change that I would suggest. Your model is very good, but a bunch of the work is still ultimately performed by the DissolveWindow::on_SpeciesScaleChargesAction_triggered function (which is the way it's always been). I think it would be worth adding a function to the model where you pass in a reference/pointer to the species and it does all the scaling within the model. That gives us a couple of advantages:
- It's now possible (and recommended) two write a unit test to confirm that the model performs the correct scaling.
- There's less code in the menu GUI, which should really only be for dispatch
- When we replace the main GUI with QML, there's no concern that we won't translate the logic correctly
There was a problem hiding this comment.
Looking good, and I think @rprospero makes a convincing case for moving the functionality into the model.
Have added some other comments, but notice that in the screengrab the buttons are aligned bottom-left in the dialog, when convention suggests bottom-right is the correct location.
RobBuchananCompPhys
force-pushed
the
scale-charges-dialog-qml-translation
branch
from
ba80920
to
480d50a
Compare
|
I've added a unit test for the model. This should be good to go. One outstanding issue is that user manually resizing the dialog's window doesn't seem cause the contained UI components to adapt their size/positioning, or enforce a minimum size. (Perhaps I'm missing something @rprospero?)
|
|
The dialog looks really nice now - great work on the harmonisation of styles! One small thing - the |
…in model, move scale charge callback functionality into model
…w void, reaaranged position of cancel button
RobBuchananCompPhys
force-pushed
the
scale-charges-dialog-qml-translation
branch
from
b0ae9b0
to
7591342
Compare
| ScaleChargesDialog::~ScaleChargesDialog() {} | ||
| ScaleChargesDialog::ScaleChargesDialog(QWidget *parent) : QDialog(parent) | ||
| { | ||
| QQuickWidget *view = new QQuickWidget(QUrl("qrc:/dialogs/qml/ScaleChargesDialog.qml"), this); |
There was a problem hiding this comment.
| QQuickWidget *view = new QQuickWidget(QUrl("qrc:/dialogs/qml/ScaleChargesDialog.qml"), this); | |
| auto view = new QQuickWidget(QUrl("qrc:/dialogs/qml/ScaleChargesDialog.qml"), this); |
| } | ||
| view->setResizeMode(QQuickWidget::SizeRootObjectToView); | ||
|
|
||
| QHBoxLayout *topLeftLayout = new QHBoxLayout; |
There was a problem hiding this comment.
| QHBoxLayout *topLeftLayout = new QHBoxLayout; | |
| auto topLeftLayout = new QHBoxLayout; |
|
|
||
| ScaleChargesDialogModel::Option ScaleChargesDialogModel::getOption() { return scaleType_; } | ||
|
|
||
| bool ScaleChargesDialogModel::scaleCharges(Species *species, bool showDialogOnError = true) |
There was a problem hiding this comment.
| bool ScaleChargesDialogModel::scaleCharges(Species *species, bool showDialogOnError = true) | |
| bool ScaleChargesDialogModel::scaleCharges(Species *species, bool showDialogOnError) |
| // Returns the current scale value | ||
| double value() const; | ||
| // Apply scaling operation to species atoms | ||
| bool scaleCharges(Species *, bool); |
There was a problem hiding this comment.
| bool scaleCharges(Species *, bool); | |
| bool scaleCharges(Species *species, bool showDialogOnError = true); |
After selecting an atomic species and clicking "Scale Charges", you should see something like this:
(The spin box is narrower than before with xml, but maybe that is alright? - the inputs aren't expected to get too large)