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

R Syntax: Load QML Controls as QML Module #5776

Open
wants to merge 14 commits into
base: development
Choose a base branch
from

Conversation

boutinb
Copy link
Contributor

@boutinb boutinb commented Jan 10, 2025

Make the QML Components as a QML Module (by using the qt_add_qml_module cmake function).
This makes also possible to use this module as a plugin: this is needed for the R Syntax project

All versions in QML import statement must be removed.

No qmldir needed (it is generated automatically), and no need to register all the base QML objects: they are automatically registered via the QML_ELEMENT macro.

The module is linked to the CommonData and has access to all DataSet objects, but it should not use static members (as the sqlite database is instantiated in another translation unit, any call to this instance would crash). So a DatasetInterface must be created to load the sqlite database, and a new DataSet must use this DatasetInterface object: (as in the Engine). The Audit analyses creates Filter object: this object must be created via this DataSet object.
The Consumer/Provider pattern that was used to read data for the QML controls, could be in the future replaced by calls to this DataSet object.

@boutinb boutinb changed the title Load QML Controls as QML Module R Syntax: Load QML Controls as QML Module Jan 13, 2025
@boutinb boutinb requested a review from JorisGoosen January 13, 2025 15:26
The QML Module is also a plugin.

All versions in QML import statement must be removed.
Remove the CommonData to the QML Comnponent.
Only the Audit analyses use the CommonData objects (via the Filter object), but this leads to unexpected error: the sqlite library is added in the translation unit of the QML Component library.
Furthermore, n order to use the Filter object in the QML Component the whole DataSet object was exposed to the QML Component, which is completely inconsistent with the Provider/Consumer pattern used in the VariableInfo.
So in order to still be able to use the Filter object in the Audit analyses, callback functions are used instead, which are set in the DesktopCommunicator object.
@shun2wang
Copy link
Contributor

shun2wang commented Jan 14, 2025

Sorry, but version things still not be removed in Description.qml Upgrades.qml in submodules I think?

Use QML_ELEMENT
Remove import version in other QML files
@boutinb
Copy link
Contributor Author

boutinb commented Jan 14, 2025

Sorry, but version things still not be removed in Description.qml Upgrades.qml in submodules I think?

That's true, I forgot these ones also.

@@ -3,6 +3,8 @@
#include "dataset.h"
#include "databaseinterface.h"

std::map<std::string, Filter*> _filterMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a terrible Idea, a global really?

Copy link
Contributor Author

@boutinb boutinb Jan 14, 2025

Choose a reason for hiding this comment

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

OK, I have changed this, by creating a new DatabaseInterface that loads the sqlite database. This is indeed much cleaner.

@@ -619,3 +619,9 @@ bool DataSet::initColumnWithStrings(int colIndex, const std::string & newName, c
return anyChanges || column->type() != prevType;
}

bool DataSet::isFilterNameFree(const std::string& filterName)
{
return -1 == db().filterGetId(filterName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes more sense to have this on Filter because the name is independent of the DataSet in question.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make it non-static?

@@ -170,11 +170,6 @@ bool Filter::checkForUpdates()
return false;
}

bool Filter::filterNameIsFree(const std::string &filterName)
{
return -1 == DatabaseInterface::singleton()->filterGetId(filterName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Which is also where it was...

@@ -68,5 +70,12 @@ bool VariableInfo::dataAvailable()

DataSet *VariableInfo::dataSet()
{
return _provider ? reinterpret_cast<DataSet*>(_provider->provideInfo(VariableInfo::DataSetPointer).value<void*>()) : nullptr;
static DataSet* singleDataSet = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to work is it?
How do you reflect the dataset getting destroyed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so as I suspected:

  • open dataset, use a sampling audit analysis, it works
  • close dataset, dont close jasp
  • open other dataset, use a sampling audit analysis, its broken...

Ill just make sure the datasetpointer works, or Ill fix it some other way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I forgot this case.

@JorisGoosen JorisGoosen force-pushed the useQMLModule branch 2 times, most recently from d40db89 to 5560bed Compare January 15, 2025 11:50
…o inside singleton() (but without creating the actual tablestructure, this remains the prerogative of DataSetPackage)
@JorisGoosen
Copy link
Contributor

So ive reverted a bunch of the changes you made to cpp code that was unnecessary.
It now runs fine on macos, I will proceed to test Windows and Linux.

linux compiles and runs
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.

3 participants