Sorted out issues with ownership between ContextMenuController and IdentifyController. Added a method to cleanup popup elements whenever the UI is closed instead of waiting until the next Identify is performed.

Author: dtedder

Handheld/qml/main.qml

@@ -450,6 +450,7 @@ Handheld {
 
                 closeCallback: () => {
                     identifyResultsContainer.visible = false;
+                    identifyController.clearPopups();
                 }
             }
 

Shared/ContextMenuController.cpp

@@ -275,21 +275,24 @@ void ContextMenuController::clearOptions()
 {
   m_options->setStringList(QStringList{});
 
-  for (const auto& geoElements : std::as_const(m_contextGeoElements))
+  for (const QList<GeoElement*>& geoElements : std::as_const(m_contextGeoElements))
   {
     if (geoElements.empty())
       continue;
 
-    // DynamicEntity and DynamicEntityObservation objects should not be deleted
-    // They are owned by their MessageFeed
-    const GeoElement* ge = geoElements.first();
-    if (const auto* de = dynamic_cast<const DynamicEntity*>(ge); de)
-      continue;
-
-    if (const auto* deo = dynamic_cast<const DynamicEntityObservation*>(ge); deo)
-      continue;
+    for (GeoElement* ge : geoElements)
+    {
+      if (!ge)
+        continue;
 
-    qDeleteAll(geoElements);
+      // anything that was previously identified from the view
+      // that is owned by the controller should be deleted
+      if (QObject* o = GeoElementUtils::toQObject(ge); o)
+      {
+        if (o->parent() == this)
+          o->deleteLater();
+      }
+    }
   }
   m_contextGeoElements.clear();
 }
@@ -363,14 +366,14 @@ void ContextMenuController::invokeIdentifyOnGeoView()
   auto idenfityLayers = geoView->identifyLayersAsync(m_contextScreenPosition, 5.0, false, -1, this);
   auto identifyGraphicsOverlays = geoView->identifyGraphicsOverlaysAsync(m_contextScreenPosition, 5.0, false, -1, this);
 
-  QtFuture::whenAll(idenfityLayers, identifyGraphicsOverlays).then(this, [this](const QList<IdentifyResultsVariant::FutureType> &identifyResults)
+  QtFuture::whenAll(idenfityLayers, identifyGraphicsOverlays).then(this, [this](const QList<IdentifyResultsVariant::FutureType>& identifyResults)
   {
     for (const IdentifyResultsVariant::FutureType& identifyResult : identifyResults)
     {
       if (identifyResult.index() == IdentifyResultsVariant::Types::LAYERS)
       {
-        LayerResultsManager resultsManager(std::get<IdentifyResultsVariant::Types::LAYERS>(identifyResult).result());
-        for (IdentifyLayerResult* result : std::as_const(resultsManager.m_results))
+        const QList<Esri::ArcGISRuntime::IdentifyLayerResult*> results = std::get<IdentifyResultsVariant::Types::LAYERS>(identifyResult).result();
+        for (IdentifyLayerResult* result : results)
         {
           if (!result)
             continue;
@@ -379,34 +382,42 @@ void ContextMenuController::invokeIdentifyOnGeoView()
           if (geoElementsAll.isEmpty())
             continue;
 
+          QList<GeoElement*> geoElementsToOwn{};
           QList<GeoElement*> geoElements{};
-          std::for_each(std::begin(geoElementsAll), std::end(geoElementsAll), [&](GeoElement* ge)
+          for (GeoElement* ge : geoElementsAll)
           {
+            // any non-observations (Feature, etc) should be owned
             DynamicEntityObservation* deo = dynamic_cast<DynamicEntityObservation*>(ge);
             if (!deo)
             {
+              geoElementsToOwn.append(ge);
               geoElements.append(ge);
-              return;
+              continue;
             }
 
-            // add any observations that are not the latest to the list to be preserved
+            // observations other than the latest at the time of the click
+            // should also be owned
             DynamicEntity* de = deo->dynamicEntity();
             if (de->latestObservation()->observationId() != deo->observationId())
             {
+              geoElementsToOwn.append(deo);
               geoElements.append(deo);
-              return;
+              continue;
             }
 
-            // for latest observations, add the entity itself and mark the observation as deleted
+            // the remaining case is that the observation was itself the latest
+            // so we add the dynamic entity instead of the observation element
+            // and mark the observation as no longer needed
             geoElements.append(de);
             deo->deleteLater();
-          });
+          }
 
-          // set the GeoElements to be managed by the tool
-          GeoElementUtils::setParent(geoElements, this);
+          // set the observations and any other non-DynamicEntity GeoElements to be owned by the tool
+          GeoElementUtils::setParent(geoElementsToOwn, this);
 
           // add the geoElements to the context hash using the layer name as the key
           m_contextGeoElements.insert(result->layerContent()->name(), geoElements);
+          result->deleteLater();
         }
       }
       else if (identifyResult.index() == IdentifyResultsVariant::Types::GRAPHICS)
@@ -492,7 +503,12 @@ void ContextMenuController::selectOption(const QString& option)
     if (!identifyTool)
       return;
 
+    // transfer the geoelements to the identify controller which takes ownership of them
     identifyTool->showPopups(m_contextGeoElements);
+
+    // clear the list so that the elements will not be cleaned up by subsequent
+    // long presses on the geoview
+    m_contextGeoElements.clear();
   }
   else if (option == OPTION_VIEWSHED)
   {

Shared/IdentifyController.cpp

@@ -34,6 +34,7 @@
 #include "PopupTypes.h"
 
 // DSA headers
+#include "GeoElementUtils.h"
 #include "ToolManager.h"
 
 
@@ -54,11 +55,12 @@ QString IdentifyController::toolName() const
   return QStringLiteral("identify");
 }
 
+/*!
+ * \brief Creates popups for the GeoElements and takes ownership of any non-DynamicEntities.
+ */
 void IdentifyController::showPopups(const QHash<QString, QList<GeoElement*>>& geoElementsByTitle)
 {
-  // reset the popups container properties
-  m_popups.clear();
-  m_currentPopupIndex = -1;
+  clearPopups();
 
   if (geoElementsByTitle.isEmpty())
   {
@@ -97,6 +99,16 @@ void IdentifyController::prevPopup()
   emit popupChanged();
 }
 
+void IdentifyController::clearPopups()
+{
+  for (Popup* popup : m_popups)
+    popup->deleteLater();
+
+  m_popups.clear();
+
+  m_currentPopupIndex = -1;
+}
+
 bool isObjectIdFieldName(QStringView fieldName)
 {
   static constexpr std::array<std::string_view, 3> oidFieldNames{
@@ -118,35 +130,42 @@ bool IdentifyController::addGeoElementPopup(GeoElement* geoElement, const QStrin
   if (!geoElement->attributes() || geoElement->attributes()->isEmpty())
     return false;
 
-  bool isDynamicEntity = false;
-  if (const auto* de = dynamic_cast<DynamicEntity*>(geoElement); de)
-    isDynamicEntity = true;
+  // default popup title to the layer name
+  auto* newPopup = new Popup(geoElement, this);
+  newPopup->popupDefinition()->setTitle(popupTitle);
+  if (const auto* de = dynamic_cast<DynamicEntity*>(geoElement); !de)
+  {
+    // set any non-dynamic entity geoelements to be owned by their popup which are cleaned up
+    GeoElementUtils::toQObject(geoElement)->setParent(newPopup);
+  }
+  else
+  {
+    // don't change the parent but set unique label indicating the latest observation
+    newPopup->popupDefinition()->setTitle(QString{"%1<br>(Live Updates)"}.arg(popupTitle));
+  }
 
-  // create a new Popup from the geoElement
-  auto newPopup = std::make_unique<Popup>(geoElement);
-  newPopup->popupDefinition()->setTitle(isDynamicEntity ? QString{"%1<br>(Live Updates)"}.arg(popupTitle) : popupTitle);
-  const auto popupElements = newPopup->popupDefinition()->elements();
-  for (const auto popupElement : popupElements)
+  const QList<PopupElement*> popupElements = newPopup->popupDefinition()->elements();
+  for (const PopupElement* popupElement : popupElements)
   {
     if (popupElement->popupElementType() != PopupElementType::FieldsPopupElement)
       continue;
 
-    const auto* fieldsPE = static_cast<FieldsPopupElement*>(popupElement);
-    const auto* fields = fieldsPE->fields();
+    const auto* fieldsPE = static_cast<const FieldsPopupElement*>(popupElement);
+    const PopupFieldListModel* fields = fieldsPE->fields();
     for (PopupField* field : *fields)
     {
       // check any fields that are not editable for common ObjectID field names
       if (!field->isEditable() && isObjectIdFieldName(field->fieldName()))
         continue;
 
       // set parent to Popup
-      auto popupFF = new PopupFieldFormat(dynamic_cast<QObject*>(newPopup.get()));
+      auto popupFF = new PopupFieldFormat(static_cast<QObject*>(newPopup));
       popupFF->setDecimalPlaces(2);
       popupFF->setUseThousandsSeparator(true);
       field->setFormat(popupFF);
     }
   }
-  m_popups.emplace_back(std::move(newPopup));
+  m_popups.push_back(newPopup);
 
   return true;
 }
@@ -166,7 +185,7 @@ Popup* IdentifyController::popup() const
   if (m_currentPopupIndex < 0 || m_currentPopupIndex >= static_cast<int>(m_popups.size()))
     return nullptr;
 
-  return m_popups.at(m_currentPopupIndex).get();
+  return m_popups.at(m_currentPopupIndex);
 }
 
 } // Dsa

Shared/IdentifyController.h

@@ -60,6 +60,7 @@ class IdentifyController : public AbstractTool
   void showPopups(const QHash<QString, QList<Esri::ArcGISRuntime::GeoElement*>>& geoElementsByTitle);
   Q_INVOKABLE void nextPopup();
   Q_INVOKABLE void prevPopup();
+  Q_INVOKABLE void clearPopups();
 
 signals:
   void popupChanged();
@@ -72,7 +73,7 @@ class IdentifyController : public AbstractTool
 
   double m_tolerance = 5.0;
   int m_currentPopupIndex = -1;
-  std::vector<std::unique_ptr<Esri::ArcGISRuntime::Popup>> m_popups;
+  std::vector<Esri::ArcGISRuntime::Popup*> m_popups;
 };
 
 // some shortcuts for working with multiple identify operation futures in a single 'QtFuture::whenAll' handler

Vehicle/qml/main.qml

@@ -443,6 +443,7 @@ Vehicle {
 
                 closeCallback: () => {
                     identifyResultsContainer.visible = false;
+                    identifyController.clearPopups();
                 }
             }