Skip to content

Commit

Permalink
Merge pull request #37186 from mantidproject/37129_parse_name_in_nest…
Browse files Browse the repository at this point in the history 
…ed_function_bug

Fix a couple property handler bugs
  • Loading branch information
@SilkeSchomann
SilkeSchomann committed May 7, 2024
2 parents 019f99b + bfaf396 commit fb34fb5
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 64 deletions.
13 changes: 2 additions & 11 deletions buildconfig/CMake/CppCheck_Suppressions.txt.in
View file
Expand Up @@ -1414,19 +1414,10 @@ constVariableReference:${CMAKE_SOURCE_DIR}/qt/widgets/plugins/algorithm_dialogs/
unreadVariable:${CMAKE_SOURCE_DIR}/qt/widgets/plugins/algorithm_dialogs/src/StartLiveDataDialog.cpp:143
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/SequentialFitDialog.cpp:108
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/SequentialFitDialog.cpp:188
passedByValue:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:1610
iterateByValue:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:1618
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:65
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:209
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:266
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:475
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:523
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:664
constVariablePointer:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:889
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:72
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:665
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:1113
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:1143
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:670
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/common/src/PropertyHandler.cpp:1118
constVariableReference:${CMAKE_SOURCE_DIR}/qt/widgets/plugins/algorithm_dialogs/src/SortTableWorkspaceDialog.cpp:115
useStlAlgorithm:${CMAKE_SOURCE_DIR}/qt/widgets/plugins/algorithm_dialogs/src/SortTableWorkspaceDialog.cpp:189
internalError:${CMAKE_SOURCE_DIR}/Framework/MDAlgorithms/src/IntegratePeaksMD2.cpp:0
Expand Down
1 change: 1 addition & 0 deletions docs/source/release/v6.10.0/Workbench/Bugfixes/37129.rst
View file
@@ -0,0 +1 @@
- Fixed a bug in the fitting property browser, where removing a function from a nested composite function could cause a crash.
2 changes: 1 addition & 1 deletion qt/widgets/common/inc/MantidQtWidgets/Common/PropertyHandler.h
View file
Expand Up @@ -178,7 +178,7 @@ class EXPORT_OPT_MANTIDQT_COMMON PropertyHandler : public QObject, public Mantid
void fit();

// update workspace property when workspaces added to or removed from ADS
void updateWorkspaces(QStringList oldWorkspaces);
void updateWorkspaces(const QStringList &oldWorkspaces);
// set workspace in workspace property to the function
void setFunctionWorkspace();

Expand Down
104 changes: 52 additions & 52 deletions qt/widgets/common/src/PropertyHandler.cpp
View file
Expand Up @@ -206,7 +206,7 @@ class CreateAttributeProperty : public Mantid::API::IFunction::ConstAttributeVis
*/
void PropertyHandler::initTies() {
for (size_t iparam = 0; iparam < m_cf->nParams(); iparam++) {
Mantid::API::ParameterTie *tie = m_cf->getTie(iparam);
const auto *tie = m_cf->getTie(iparam);
if (tie) {
// get function index from prefix (second element of pair below)
const auto nameIndex_pair = m_cf->parseName(m_cf->parameterName(iparam));
Expand Down Expand Up @@ -257,13 +257,11 @@ void PropertyHandler::initParameters() {
m_item->property()->addSubProperty(prop);
m_parameters << prop;
if (m_fun->isFixed(i)) {
QtProperty *tieProp = m_browser->m_stringManager->addProperty("Tie");
m_browser->m_stringManager->setValue(tieProp, QString::number(m_fun->getParameter(i)));
prop->addSubProperty(tieProp);
m_ties[parName] = tieProp;
fix(parName);
m_browser->m_changeSlotsEnabled = false;
}
// add constraint properties
Mantid::API::IConstraint *c = m_fun->getConstraint(i);
const Mantid::API::IConstraint *c = m_fun->getConstraint(i);
if (c) {
QStringList qc = QString::fromStdString(c->asString()).split("<");
bool lo = false;
Expand Down Expand Up @@ -472,16 +470,23 @@ void PropertyHandler::renameChildren(const Mantid::API::CompositeFunction &cf) {
QString parName = it.key();
QString fullName = functionPrefix() + "." + parName;
QtProperty *prop = it.value();
Mantid::API::ParameterTie *tie = cf.getTie(cf.parameterIndex(fullName.toStdString()));
const auto paramIndex = m_browser->compositeFunction()->parameterIndex(fullName.toStdString());
const auto status = cf.getParameterStatus(paramIndex);
const auto *tie = cf.getTie(paramIndex);
if (!tie) {
// In this case the tie has been removed from the composite function since it contained a reference to
// the function which was removed
QtProperty *parProp = getParameterProperty(parName);
if (parProp != nullptr) {
parProp->removeSubProperty(prop);
// Don't increment the iterator if we delete the current tie.
it = m_ties.erase(it);
parProp->setEnabled(true);
if (status != Mantid::API::IFunction::ParameterStatus::Fixed &&
status != Mantid::API::IFunction::ParameterStatus::FixedByDefault) {
// In this case the tie has been removed from the composite function since it contained a reference to
// the function which was removed
QtProperty *parProp = getParameterProperty(parName);
if (parProp != nullptr) {
parProp->removeSubProperty(prop);
// Don't increment the iterator if we delete the current tie.
it = m_ties.erase(it);
parProp->setEnabled(true);
}
} else {
++it;
}
continue;
} else {
Expand Down Expand Up @@ -520,7 +525,7 @@ QString PropertyHandler::functionName() const {
}

QString PropertyHandler::functionPrefix() const {
PropertyHandler *ph = parentHandler();
const PropertyHandler *ph = parentHandler();
if (ph) {
int iFun = -1;
Mantid::API::CompositeFunction_sptr cf = ph->cfun();
Expand Down Expand Up @@ -661,7 +666,7 @@ bool PropertyHandler::setParameter(QtProperty *prop) {

// If the parameter is fixed, re-fix to update the subproperty.
if (m_fun->isFixed(m_fun->parameterIndex(parName))) {
foreach (QtProperty *subProp, prop->subProperties()) {
foreach (const QtProperty *subProp, prop->subProperties()) {
if (subProp->propertyName() == "Fix") {
fix(prop->propertyName());
break;
Expand Down Expand Up @@ -886,7 +891,7 @@ void PropertyHandler::setAttribute(QString const &attName, AttributeType const &
try {
m_fun->setAttribute(attName.toStdString(), Mantid::API::IFunction::Attribute(attValue));
m_browser->compositeFunction()->checkFunction();
foreach (QtProperty *prop, m_attributes) {
foreach (const QtProperty *prop, m_attributes) {
if (prop->propertyName() == attName) {
// re-insert the attribute and parameter properties as they may
// depend on the value of the attribute being set
Expand Down Expand Up @@ -1137,47 +1142,42 @@ void PropertyHandler::addTie(const QString &tieStr) {
auto &cfunction = *m_browser->compositeFunction();
cfunction.tie(name, expr);
cfunction.applyTies();
const auto paramIndex = cfunction.parameterIndex(name);
const auto paramStatus = cfunction.getParameterStatus(paramIndex);
const bool fixed = paramStatus == Mantid::API::IFunction::ParameterStatus::Fixed;
const bool recursive = true;
QString parName = QString::fromStdString(cfunction.parameterLocalName(cfunction.parameterIndex(name), recursive));
foreach (QtProperty *parProp, m_parameters) {
if (parProp->propertyName() == parName) {
m_browser->m_changeSlotsEnabled = false;
QtProperty *tieProp = m_ties[parName];
if (!tieProp) {
tieProp = m_browser->m_stringManager->addProperty("Tie");
m_ties[parName] = tieProp;
}
m_browser->m_stringManager->setValue(tieProp, QString::fromStdString(expr));
m_browser->m_changeSlotsEnabled = true;
parProp->addSubProperty(tieProp);
updateParameters();
return;
}
QString parName = QString::fromStdString(cfunction.parameterLocalName(paramIndex, recursive));
QtProperty *parProp = getParameterProperty(parName);
if (!parProp)
return;
m_browser->m_changeSlotsEnabled = false;
QtProperty *tieProp = m_ties[parName];
if (!tieProp) {
const auto tiePropName = fixed ? "Fix" : "Tie";
tieProp = m_browser->m_stringManager->addProperty(tiePropName);
m_ties[parName] = tieProp;
}
} catch (...) {
m_browser->m_stringManager->setValue(tieProp, QString::fromStdString(expr));
parProp->addSubProperty(tieProp);
if (fixed) {
tieProp->setEnabled(false);
}
m_browser->m_changeSlotsEnabled = true;
if (!fixed) {
updateParameters();
}
} catch (const std::exception &exc) {
std::cerr << exc.what();
QMessageBox::critical(m_browser, "Mantid - Error", "Failed to set tie: " + tieStr);
}
QMessageBox::critical(m_browser, "Mantid - Error", "Failed to set tie: " + tieStr);
}

void PropertyHandler::fix(const QString &parName) {
QtProperty *parProp = getParameterProperty(parName);
const QtProperty *parProp = getParameterProperty(parName);
if (!parProp)
return;
QString parValue = QString::number(m_browser->m_parameterManager->value(parProp));
try {
m_fun->tie(parName.toStdString(), parValue.toStdString());
m_browser->m_changeSlotsEnabled = false;
QtProperty *tieProp = m_ties[parName];
if (!tieProp) {
tieProp = m_browser->m_stringManager->addProperty("Fix");
m_ties[parName] = tieProp;
}
m_browser->m_stringManager->setValue(tieProp, parValue);
m_browser->m_changeSlotsEnabled = true;
parProp->addSubProperty(tieProp);
tieProp->setEnabled(false);
} catch (...) {
}
addTie(functionPrefix() + "." + parName + "=" + parValue);
}

/**
Expand Down Expand Up @@ -1607,15 +1607,15 @@ void PropertyHandler::fit() {
}
}

void PropertyHandler::updateWorkspaces(QStringList oldWorkspaces) {
void PropertyHandler::updateWorkspaces(const QStringList &oldWorkspaces) {
if (m_workspace) {
int index = m_browser->m_enumManager->value(m_workspace) - 1;
QString wsName;
if (index >= 0 && index < oldWorkspaces.size()) {
wsName = oldWorkspaces[index];
}
QStringList names("All");
foreach (QString name, m_browser->m_workspaceNames) { names.append(name); }
foreach (const QString &name, m_browser->m_workspaceNames) { names.append(name); }
m_browser->m_enumManager->setEnumNames(m_workspace, names);
if (m_browser->m_workspaceNames.contains(wsName)) {
m_browser->m_enumManager->setValue(m_workspace, m_browser->m_workspaceNames.indexOf(wsName) + 1);
Expand Down

0 comments on commit fb34fb5

Please sign in to comment.