From 51437d09f5f99b0304f3ac29a74038eddcb27982 Mon Sep 17 00:00:00 2001 From: bruno boutin Date: Wed, 13 Apr 2022 16:08:28 +0200 Subject: [PATCH] The source of a Dropdown should be fetched before its value is set when reading a JASP file Fixes https://github.com/jasp-stats/jasp-test-release/issues/1780 This solves 2 problems: . a dropdown with a source is not set properly when a JASP file is read: if the dropdown was set with a value coming from the source, when reading the JASP file, if the values of the source are not yet read, setting the dropdown generates an error. This is the case in the ANOVA analysis: the 'Reference model for weight ratios' dropdown in the Order Restriction, has for source the titles of the tabs of the Order Restriction models. If the dropdown is set with one of these values, reading a JASP file with this setting will lead to an error. . if an error is set to a control, removing this error lets the analysis is an error state, and the analysis cannot run anymore. In AnalysisForm, the terms of an available model (which is the case for a dropdown) should be fetched before the value of the dropdown is set. Also remove the _getControlErrorMessageOfControl method which is not used anymore. In ControlErrorMessage, when the message is closed, its association with its control should be removed: in this way the AnalysisForm::hasError becomes false. --- Desktop/analysis/analysisform.cpp | 49 +++++++------------ Desktop/analysis/analysisform.h | 1 - .../JASP/Widgets/ControlErrorMessage.qml | 1 + 3 files changed, 19 insertions(+), 32 deletions(-) diff --git a/Desktop/analysis/analysisform.cpp b/Desktop/analysis/analysisform.cpp index 2ac43e7518..b8cea470a7 100644 --- a/Desktop/analysis/analysisform.cpp +++ b/Desktop/analysis/analysisform.cpp @@ -362,6 +362,24 @@ void AnalysisForm::bindTo() BoundControl* boundControl = control->boundControl(); JASPListControl* listControl = dynamic_cast(control); + if (listControl && listControl->hasSource()) + { + ListModelAvailableInterface* availableModel = qobject_cast(listControl->model()); + // The availableList control are not bound with options, but they have to be updated from their sources when the form is initialized. + // The availableList cannot signal their assigned models now because they are not yet bound (the controls are ordered by dependency) + // When the options come from a JASP file, an assigned model needs sometimes the available model (eg. to determine the kind of terms they have). + // So in this case resetTermsFromSourceModels has to be called now but with updateAssigned argument set to false. + // When the options come from default options (from source models), the availableList needs sometimes to signal their assigned models (e.g. when addAvailableVariablesToAssigned if true). + // As their assigned models are not yet bound, resetTermsFromSourceModels (with updateAssigned argument set to true) must be called afterwards. + if (availableModel) + { + if (defaultOptions != Json::nullValue || _analysis->isDuplicate()) + availableModel->resetTermsFromSources(false); + else + availableModelsToBeReset.push_back(availableModel); + } + } + if (boundControl) { std::string name = control->name().toStdString(); @@ -382,24 +400,6 @@ void AnalysisForm::bindTo() boundControl->bindTo(optionValue); } - if (listControl && listControl->hasSource()) - { - ListModelAvailableInterface* availableModel = qobject_cast(listControl->model()); - // The availableList control are not bound with options, but they have to be updated from their sources when the form is initialized. - // The availableList cannot signal their assigned models now because they are not yet bound (the controls are ordered by dependency) - // When the options come from a JASP file, an assigned model needs sometimes the available model (eg. to determine the kind of terms they have). - // So in this case resetTermsFromSourceModels has to be called now but with updateAssigned argument set to false. - // When the options come from default options (from source models), the availableList needs sometimes to signal their assigned models (e.g. when addAvailableVariablesToAssigned if true). - // As their assigned models are not yet bound, resetTermsFromSourceModels (with updateAssigned argument set to true) must be called afterwards. - if (availableModel) - { - if (defaultOptions != Json::nullValue || _analysis->isDuplicate()) - availableModel->resetTermsFromSources(false); - else - availableModelsToBeReset.push_back(availableModel); - } - } - control->setInitialized(hasOption); } @@ -437,19 +437,6 @@ void AnalysisForm::addFormError(const QString &error) emit errorsChanged(); } -QQuickItem* AnalysisForm::_getControlErrorMessageOfControl(JASPControl* jaspControl) -{ - QQuickItem* result = nullptr; - - for (QQuickItem* item : _controlErrorMessageCache) - if (item->parentItem() == jaspControl) - { - result = item; - break; - } - - return result; -} //This should be moved to JASPControl maybe? //Maybe even to full QML? Why don't we just use a loader... diff --git a/Desktop/analysis/analysisform.h b/Desktop/analysis/analysisform.h index b7d8fd140b..2cad8f3d7c 100644 --- a/Desktop/analysis/analysisform.h +++ b/Desktop/analysis/analysisform.h @@ -157,7 +157,6 @@ public slots: void setControlMustContain( QString controlName, QStringList containThis); void setControlIsDependency( std::string controlName, bool isDependency) { setControlIsDependency(tq(controlName), isDependency); } void setControlMustContain( std::string controlName, std::set containThis) { setControlMustContain(tq(controlName), tql(containThis)); } - QQuickItem* _getControlErrorMessageOfControl(JASPControl* jaspControl); void setAnalysisUp(); std::vector > _getValuesFromJson(const Json::Value& jsonValues, const QStringList& searchPath); diff --git a/Desktop/components/JASP/Widgets/ControlErrorMessage.qml b/Desktop/components/JASP/Widgets/ControlErrorMessage.qml index 01ff7b967b..2791ff4913 100644 --- a/Desktop/components/JASP/Widgets/ControlErrorMessage.qml +++ b/Desktop/components/JASP/Widgets/ControlErrorMessage.qml @@ -47,6 +47,7 @@ Rectangle { control.hasError = false; control.hasWarning = false; + control = null; parent = null; }