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

Modify charge dialogs qml #1787

Merged
merged 21 commits into from Feb 9, 2024
Merged

Modify charge dialogs qml #1787

merged 21 commits into from Feb 9, 2024

Conversation

RobBuchananCompPhys
Copy link
Contributor

@RobBuchananCompPhys RobBuchananCompPhys commented Feb 1, 2024

This PR aims to implement the three dialogs "Scale Charges", "Smooth Charges" and "Reduce Sig Figs in Charges", in QML.
I have used a generic ModifyChargesModel to handle the resources corresponding to each operation, since all dialogs share a common, simple format of components, and similarly updated the dialog class to handle connections between all three QMLs and the model.
Therefore, this supersedes (and incorporates) the work I did recently on the ScaleChargesDialog/Model.

@trisyoungs
Copy link
Member

Something corrupt / missing in the qml files, or at least their location as I get this error (as does the CI):
ninja: error: '/home/tris/src/dissolve/src/gui/qml/ModifyChargesDialog.qml', needed by 'src/gui/qrc_main.cpp', missing and no known rule to make it

@trisyoungs trisyoungs mentioned this pull request Feb 2, 2024
src/gui/menu_species.cpp Outdated Show resolved Hide resolved
src/gui/modifyChargesDialog.h Show resolved Hide resolved
src/gui/models/modifyChargesModel.h Outdated Show resolved Hide resolved
src/gui/models/modifyChargesModel.h Outdated Show resolved Hide resolved
src/gui/qml/modifyCharges/Layout.qml Outdated Show resolved Hide resolved
src/main/dissolve.h Outdated Show resolved Hide resolved
@trisyoungs
Copy link
Member

@RobBuchananCompPhys An update from my machine. Upgrading my distribution has now furnished me with Qt 6.4.2 which solves all of the odd errors with text / widget colouring I was seeing. I also now understand the font scaling issue - I still see odd results when setting font.pixelSize but font.pointSize gives the expected results when set to 11. This must be down to a mis-calculation of screen DPI or something similar at the Qt end since my display is not scaled, but it seems that the recommended approach is to use pointSize wherever possible.

@trisyoungs
Copy link
Member

@RobBuchananCompPhys An update from my machine. Upgrading my distribution has now furnished me with Qt 6.4.2 which solves all of the odd errors with text / widget colouring I was seeing. I also now understand the font scaling issue - I still see odd results when setting font.pixelSize but font.pointSize gives the expected results when set to 11. This must be down to a mis-calculation of screen DPI or something similar at the Qt end since my display is not scaled, but it seems that the recommended approach is to use pointSize wherever possible.

We can set this in the base D.Text class, presumably, in order to provide a consistent result?

@RobBuchananCompPhys RobBuchananCompPhys marked this pull request as ready for review February 6, 2024 08:06
Copy link
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

Looking good. Like the unit test!

I still have a question over whether we really need to stash a pointer to a ModifyChargesModel anywhere - rather, can't we just new one when we need it (see my comment)?

src/gui/menu_species.cpp Outdated Show resolved Hide resolved
src/gui/menu_species.cpp Outdated Show resolved Hide resolved
src/gui/models/modifyChargesModel.cpp Outdated Show resolved Hide resolved
src/gui/modifyChargesDialog.cpp Outdated Show resolved Hide resolved
Copy link
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

Looking very nice now. Regarding the font.pointSize issue, do we then just remove that for now until we understand the (cross-platform) effects a bit better?

Also, I have pushed a small commit on to the PR which just moves the declaration of the model in gui.h to more of its own section. Felt silly explaining that and requesting a change!

@RobBuchananCompPhys
Copy link
Contributor Author

RobBuchananCompPhys commented Feb 8, 2024

Looking very nice now. Regarding the font.pointSize issue, do we then just remove that for now until we understand the (cross-platform) effects a bit better?

Also, I have pushed a small commit on to the PR which just moves the declaration of the model in gui.h to more of its own section. Felt silly explaining that and requesting a change!

Thanks @trisyoungs !
I have asked that @rprospero takes a look at how this looks post-font.pointSize: 11 commit just to get another opinion, currently sounds like it may well be a graphics issue. But I'd certainly consider holding out and reverting that particular commit if we can't pin down exactly why the odd size changes are occurring on diff platforms.

On another note, I noticed that the simulation menu also utilises Reduce significant figures in charges... function, so I'll add a new commit which essentially does the same thing in the species menu for sig figs. We can maybe discuss that one more in stand up tomorrow.

@rprospero
Copy link
Contributor

@trisyoungs @RobBuchananCompPhys Here's the dialog on my machine.

dialog

@RobBuchananCompPhys
Copy link
Contributor Author

@trisyoungs @RobBuchananCompPhys Here's the dialog on my machine.

dialog

Looks promising! Shall we merge?

@rprospero rprospero merged commit bd6df12 into develop Feb 9, 2024
10 checks passed
@rprospero rprospero deleted the modify_charge_dialogs_qml branch February 9, 2024 14:16
@rprospero rprospero restored the modify_charge_dialogs_qml branch February 11, 2024 20:07
@trisyoungs trisyoungs added this to the Release 1.5 milestone Feb 19, 2024
rprospero pushed a commit that referenced this pull request Mar 11, 2024
Co-authored-by: Tristan Youngs <tristan.youngs@stfc.ac.uk>
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