Fix a couple property handler bugs

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -1464,19 +1464,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

docs/source/release/v6.10.0/Workbench/Bugfixes/37129.rst

@@ -0,0 +1 @@
+- Fixed a bug in the fitting property browser, where removing a function from a nested composite function could cause a crash.

qt/widgets/common/inc/MantidQtWidgets/Common/PropertyHandler.h

@@ -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();
 

qt/widgets/common/src/PropertyHandler.cpp

@@ -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));
@@ -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;
@@ -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 {
@@ -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();
@@ -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;
@@ -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
@@ -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);
 }
 
 /**
@@ -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);