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: Fully QML GUI as separate target executable #1894

Merged
merged 16 commits into from
May 17, 2024
Merged

Conversation

RobBuchananCompPhys
Copy link
Contributor

@RobBuchananCompPhys RobBuchananCompPhys commented May 10, 2024

This PR takes on board the comments and discussion over the previous attempt to start a QML GUI for Dissolve.
The idea is now to go with a separate executable, with entrypoint in dissolve-gui-qml.
A couple of lines have been commented out in main where the dissolveWindow object was used while I figure out how to proceed with the CLI options parsing using a pure QML window, rather than Qt Widgets (basically, trying to make it work for now).
Additionally, the CMake files almost certainly link far more dependencies for the new QML-based executable than is necessary, so over time we should aim to identify what is not needed and remove from the list.

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.

I haven't had a chance to take a good look at the actual C++ (I'll do that after lunch), but this rewrite is a great opportunity to drop a ton of link time dependencies that we won't really need. I've gone through and pruned it down a bit, with some suggestions on how to do a little more.

src/dissolve-gui-qml.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
src/gui-qml/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui-qml/CMakeLists.txt Outdated Show resolved Hide resolved
src/gui-qml/CMakeLists.txt Outdated Show resolved Hide resolved
@RobBuchananCompPhys RobBuchananCompPhys marked this pull request as ready for review May 14, 2024 08:43
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.

Tested and working well on Linux. 👍

@RobBuchananCompPhys
Copy link
Contributor Author

Tested and working well on Linux. 👍

Great, thanks for looking into it. @rprospero do we need to look at restoring some of those dependencies to fix the macos build?

@rprospero
Copy link
Contributor

Just a record of where I'm at on this.

  1. For some reason, on the Mac build, and only the Mac build, the QML gui depends on our render target.
  2. The render target depends on FTGL.
  3. The render target also depends on the StockColours class, which is in the gui target.
  4. Having the QML GUI depend on gui means brining in all of the other dependencies.

There's three possible routes forward from here

  1. Try and remove the Apple dependency on the render target.
  2. Split the classes required by render out of gui and into a separate target.
  3. Abandon all hope and just link all the dependencies back into the QML gui.

I've listed the options in the order that I will attempt them.

@rprospero rprospero requested a review from trisyoungs May 16, 2024 12:16
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.

Looks like an excellent starting point.

Comment on lines 71 to 72
if (options.inputFile())
// loadSuccessful = dissolveWindow.loadInputFile(options.inputFile().value());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we've commented out the actual action, this if statement will act on the next statement. Granted, that is also another conditional that tests the same condition, but it might be better to comment out or remove the condition.

Copy link
Member

Choose a reason for hiding this comment

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

We'll need it eventually probably, so I'd vote for commenting the condition out.

Comment on lines +52 to +62
// Ensure that the C locale is set, otherwise printf() and friends may not use dot for the radix point
setlocale(LC_NUMERIC, "C");
QLocale::setDefault(QLocale::C);

// Print GPL license information
Messenger::print("Dissolve-GUI-QML {} version {}, Copyright (C) 2024 Team Dissolve and contributors.\n", Version::appType(),
Version::info());
Messenger::print("Source repository: {}.\n", Version::repoUrl());
Messenger::print("Dissolve comes with ABSOLUTELY NO WARRANTY.\n");
Messenger::print("This is free software, and you are welcome to redistribute it under certain conditions.\n");
Messenger::print("For more details read the GPL at <http://www.gnu.org/copyleft/gpl.html>.\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to put this boilerplate for in a method on the Dissolve class. You could pass in the program name and it could do this generic setup. Granted, we don't have to do it on this PR, but it might be worth flagging the issue.

@rprospero rprospero merged commit 01db2d9 into develop May 17, 2024
10 checks passed
@rprospero rprospero deleted the qml-gui-target branch May 17, 2024 14:47
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