Adding Validate utility network topology sample #1689

har13205 · 2024-03-11T23:51:14Z

Description

This PR is to add Validate utility network topology sample

Type of change

Bug fix
New sample implementation
Sample viewer enhancement
Other enhancement

Platforms tested on:

Checklist

JamesMBallard

A few things to review. See comments. Mainly, I think the general workflows should be split into smaller functions rather than lengthy connect lambdas and .then lambdas.

JamesMBallard · 2024-03-12T15:44:24Z

...Qt_CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.h

+  bool m_isUpdateVisible;
+  bool m_validateBtn;
+  bool m_traceBtn;
+  bool m_busy;
+  bool m_clearBtn;
+  bool m_stateBtn;
+  int m_attributeValue;


These all need default values.

Also, I don't understand what m_traceBtn means as a bool. Same comment for many of these bools.

Changed to m_isTraceBtnEnabled and similar for others

JamesMBallard · 2024-03-12T15:46:28Z

...Qt_CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.h

+  void setupTraceParameters();
+  void connectSignals();
+
+  Esri::ArcGISRuntime::Map* m_map = nullptr;


Do you need all these class members? I would think some of these objects are only needed temporarily, for example do you ever reference m_cred or many of these in different functions?

I would trim this list as much as reasonably possible. It gives an impression that this workflow is more complicated than necessary.

JamesMBallard · 2024-03-12T15:47:01Z

...Qt_CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.h

+  void setMapView(Esri::ArcGISRuntime::MapQuickView* mapView);
+  Esri::ArcGISRuntime::LabelDefinition* createDeviceLabelDefinition();
+  Esri::ArcGISRuntime::LabelDefinition* createLineLabelDefinition();
+  void getLabelDefinitions();


Would you consider another name for this function? It begins with 'get' but does not return anything.

changed to displayLabelDefinitions

JamesMBallard · 2024-03-12T15:48:45Z

...Qt_CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.h

+signals:
+  void mapViewChanged();
+  void messageChanged();
+  void addChoices();


This name doesn't match the naming convention for a signal. It should convey a change of some kind, like choicesChanged.

JamesMBallard · 2024-03-12T15:49:59Z

...Qt_CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.h

+  void isUpdateVisible();
+  void updateFieldName();
+  void validateBtn();
+  void traceBtn();
+  void isBusy();
+  void clearBtn();
+  void stateBtn();
+  void attributeValue();


Review these signals names. They don't follow typical guidelines for events.

JamesMBallard · 2024-03-12T16:24:51Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+
+void ValidateUtilityNetworkTopology::onValidate()
+{
+  m_utilityNetwork = m_mapView->map()->utilityNetworks()->first();


Does your logic prevent this from being called before all these are available? If not this will crash. If the service is down, for example, will the user be able to get into this logic?

added checking

JamesMBallard · 2024-03-12T16:26:51Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+    m_deviceFeatureLayer->selectFeaturesAsync(deviceParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)
+    {
+    });
+
+    m_lineFeatureLayer->selectFeaturesAsync(lineParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)
+    {
+    });


This makes sense if you don't need to do anything here, but you could clean up unused memory here.

Suggested change

m_deviceFeatureLayer->selectFeaturesAsync(deviceParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)

{

});

m_lineFeatureLayer->selectFeaturesAsync(lineParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)

{

});

m_deviceFeatureLayer->selectFeaturesAsync(deviceParams, SelectionMode::Add).then(this, [](FeatureQueryResult* rawResult)

{

std::unique_ptr<FeatureQueryResult> {rawResult};

});

m_lineFeatureLayer->selectFeaturesAsync(lineParams, SelectionMode::Add).then(this, [](FeatureQueryResult* rawResult)

{

std::unique_ptr<FeatureQueryResult> {rawResult};

});

JamesMBallard · 2024-03-12T16:27:58Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  m_utilityAssetType = m_utilityNetwork
+                           ->definition()
+                           ->networkSource("Electric Distribution Device")
+                           ->assetGroup("Circuit Breaker")
+                           ->assetType("Three Phase");


This is concise, but if anything goes wrong this will crash. Consider some minimal checking here.

JamesMBallard · 2024-03-12T16:29:52Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  return deviceLabelDefinition;
+}
+
+LabelDefinition* ValidateUtilityNetworkTopology::createLineLabelDefinition()


I think this function can be const. Everything that can be const, should be const.

JamesMBallard · 2024-03-12T16:30:01Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  emit isBusy();
+}
+
+LabelDefinition* ValidateUtilityNetworkTopology::createDeviceLabelDefinition()


Can this be const?

ldanzinger · 2024-03-19T13:57:59Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  connect(m_serviceGeodatabase, &ServiceGeodatabase::doneLoading, this, [this, params](const Esri::ArcGISRuntime::Error &loadError)
+          {
+            onServiceGeodatabaseLoaded(params, loadError);
+          });
+
+  connect(m_utilityNetwork, &UtilityNetwork::doneLoading, this, [this]()
+          {
+            setupTraceParameters();
+          });


the done loading connections should be setup before load is called. Otherwise, it's possible that the load will complete before the signal emits.

ldanzinger · 2024-03-19T13:58:18Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  ServiceVersionParameters* params = new ServiceVersionParameters(this);
+  params->setName("ValidateNetworkTopology_" + randomVersionUuid);
+  params->setAccess(VersionAccess::Private);
+  params->setDescription("Validate network topology with ArcGIS Runtime");


Suggested change

params->setDescription("Validate network topology with ArcGIS Runtime");

params->setDescription("Validate network topology with ArcGIS Maps SDK");

ldanzinger · 2024-03-19T16:01:34Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  m_updateFieldName = m_feature->featureTable()->tableName() == "Electric Distribution Device" ? "devicestatus" : "nominalvoltage";
+  Field m_field = m_feature->featureTable()->field(m_updateFieldName);
+  CodedValueDomain m_codedValueDomain = static_cast<CodedValueDomain>(m_field.domain());
+  m_codedValues = m_codedValueDomain.codedValues();


following James's comment, it still feels like more things are member variables than are probably required. In the case of Field, that is now changed to be a local variable, but it still uses the m_ convention, which suggests it's a member when it's really not. We should only make things members if we need to, and if we make them local, they shouldn't use m_

ldanzinger · 2024-03-19T16:04:01Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+
+  m_choices.clear();
+
+  m_choices.append("");


why do you need to append an empty string?

Here, to get the attributevalue of the selected feature as the current option in the choices dropdown, first I'm assigning a empty value and replacing it with the feature's attribute value when it's found in the for loop

ldanzinger · 2024-03-19T16:04:36Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  emit updateFieldName();
+}
+
+void ValidateUtilityNetworkTopology::onApplyEdits(QString choice)


Suggested change

void ValidateUtilityNetworkTopology::onApplyEdits(QString choice)

void ValidateUtilityNetworkTopology::onApplyEdits(const QString& choice)

ldanzinger · 2024-03-19T16:22:16Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+    if (m_map->loadStatus() != LoadStatus::Loaded)
+      return;
+
+    Point m_clickPoint = m_mapView->screenToLocation(mouseEvent.position().x(), mouseEvent.position().y());


this doesn't seem to be used anywhere. The identify takes the position in directly

ldanzinger · 2024-03-19T16:24:59Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+
+    // Get the validation result
+    UtilityNetworkValidationJob* job = m_utilityNetwork->validateNetworkTopology(m_extent);
+    job->start();


start should be called after the signal connection is made

ldanzinger · 2024-03-19T16:26:30Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+    UtilityNetworkValidationJob* job = m_utilityNetwork->validateNetworkTopology(m_extent);
+    job->start();
+
+    QObject::connect(job, &UtilityNetworkValidationJob::statusChanged, this, [this, job](JobStatus status)


i think we don't need to specify QObject::

Suggested change

QObject::connect(job, &UtilityNetworkValidationJob::statusChanged, this, [this, job](JobStatus status)

connect(job, &UtilityNetworkValidationJob::statusChanged, this, [this, job](JobStatus status)

ldanzinger · 2024-03-19T16:28:33Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+  SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]");
+
+  TextSymbol* textSymbol = new TextSymbol();


missing parents - any time you use new, you know you'll need to specify a parent object

Suggested change

SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]");

TextSymbol* textSymbol = new TextSymbol();

SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]", this);

TextSymbol* textSymbol = new TextSymbol(this);

ldanzinger · 2024-03-19T16:31:06Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.qml

+        id: updateWindow
+        visible: model.isUpdateWindowVisible
+        enabled: visible
+        anchors.centerIn: parent


typically we put the anchors at the top, right under the id

ldanzinger · 2024-03-19T16:45:00Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+    m_deviceFeatureLayer->clearSelection();
+    m_lineFeatureLayer->clearSelection();


we need to be careful when accessing members without a null check. I tested the branch locally, and got a crash right away. I believe it is because I tried clicking on a feature on the map before the map/utility network were fully loaded, and something was in a null state.

ldanzinger · 2024-03-20T18:22:24Z

..._CppSamples/UtilityNetwork/ValidateUtilityNetworkTopology/ValidateUtilityNetworkTopology.cpp

+
+LabelDefinition* ValidateUtilityNetworkTopology::createDeviceLabelDefinition() const
+{
+  SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]", new QObject());


all of these new QObject() need to be changed to this

Suggested change

SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]", new QObject());

SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]", this);

As per our offline discussion, removed const from the methods and parent is changed to this

ldanzinger

LGTM

har13205 added 3 commits March 11, 2024 12:17

Adding Validate Utility Network Topology sample

79caf28

updating screenshot

62727f1

Adding comments and updating portal

13d2285

har13205 requested review from ldanzinger and JamesMBallard March 11, 2024 23:51

har13205 self-assigned this Mar 11, 2024

JamesMBallard reviewed Mar 12, 2024

View reviewed changes

Changes as per PR comments

117e671

har13205 requested a review from JamesMBallard March 19, 2024 02:46

ldanzinger requested changes Mar 19, 2024

View reviewed changes

Changes as per PR comments

de112d6

har13205 requested a review from ldanzinger March 20, 2024 17:17

ldanzinger requested changes Mar 20, 2024

View reviewed changes

Removing const for methods

1b8b70d

har13205 requested a review from ldanzinger March 20, 2024 19:56

ldanzinger approved these changes Mar 20, 2024

View reviewed changes

har13205 merged commit 937a7ab into v.next Mar 21, 2024
1 check passed

har13205 deleted the har13205/validateUtilityNetworkTopology branch March 21, 2024 04:04

-    m_deviceFeatureLayer->selectFeaturesAsync(deviceParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)
-    {
-    });
-    m_lineFeatureLayer->selectFeaturesAsync(lineParams, SelectionMode::Add).then(this, [](FeatureQueryResult*)
-    {
-    });
+    m_deviceFeatureLayer->selectFeaturesAsync(deviceParams, SelectionMode::Add).then(this, [](FeatureQueryResult* rawResult)
+    {
+      std::unique_ptr<FeatureQueryResult> {rawResult};
+    });
+    m_lineFeatureLayer->selectFeaturesAsync(lineParams, SelectionMode::Add).then(this, [](FeatureQueryResult* rawResult)
+    {
+      std::unique_ptr<FeatureQueryResult> {rawResult};
+    });

	params->setDescription("Validate network topology with ArcGIS Runtime");
	params->setDescription("Validate network topology with ArcGIS Maps SDK");

	void ValidateUtilityNetworkTopology::onApplyEdits(QString choice)
	void ValidateUtilityNetworkTopology::onApplyEdits(const QString& choice)

	QObject::connect(job, &UtilityNetworkValidationJob::statusChanged, this, [this, job](JobStatus status)
	connect(job, &UtilityNetworkValidationJob::statusChanged, this, [this, job](JobStatus status)

		SimpleLabelExpression* labelExpression = new SimpleLabelExpression("[devicestatus]");

		TextSymbol* textSymbol = new TextSymbol();

		m_deviceFeatureLayer->clearSelection();
		m_lineFeatureLayer->clearSelection();

Adding Validate utility network topology sample #1689

Adding Validate utility network topology sample #1689

Conversation

har13205 commented Mar 11, 2024

Description

Type of change

Checklist

JamesMBallard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldanzinger left a comment

Choose a reason for hiding this comment