Adding Configure basemap style language sample #1688

har13205 · 2024-03-02T02:00:34Z

Description

This PR is to add Configure basemap style language sample

Type of change

Bug fix
New sample implementation
Sample viewer enhancement
Other enhancement

Platforms tested on:

Checklist

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

JamesMBallard · 2024-03-04T18:00:30Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.qml

+    function setNewBasemap() {
+        model.setNewBasemap(globalButton.checked , comboBox.currentText);


Probably both should have a clearer name.

JamesMBallard · 2024-03-04T18:02:48Z

ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/screenshot.png

The screenshot proportions don't look correct. It looks like it's been squished.

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

JamesMBallard · 2024-03-04T18:04:17Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+  basemapStyleParameters->setLanguageStrategy(global? BasemapStyleLanguageStrategy::Global : BasemapStyleLanguageStrategy::Local);
+
+  //A SpecificLanguage setting overrides the LanguageStrategy settings
+  if (language == "none") {


Review bracket style.

ldanzinger · 2024-03-04T18:48:03Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.qml

+        }
+    }
+
+    function setNewBasemap() {


seems unnecessary to have the JS function wrapping the C++ function - why not just directly call the C++ function from the combobox itself?

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

…ge/ConfigureBasemapStyleLanguage.h Co-authored-by: James Ballard <[email protected]>

…ge/ConfigureBasemapStyleLanguage.qml Co-authored-by: James Ballard <[email protected]>

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

JamesMBallard

Looking good, but I noticed a few other minor things.

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

JamesMBallard · 2024-03-05T00:24:18Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+{
+  if (!basemapStyleParameters) 
+  {
+    basemapStyleParameters = new BasemapStyleParameters(this);


It looks like this is always needed. If so, consider creating it in the constructor.

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

ldanzinger

Looking good, I've got a couple more suggestions for cleanup

...ISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.h

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

ldanzinger · 2024-03-05T14:47:10Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+  connect(m_map, &Map::doneLoading, this, [](const Error& e)
+          {
+            if (!e.isEmpty())
+            {
+              qWarning() << e.message() << e.additionalMessage();
+              return;
+            }
+          });


probably unnecessary to include checking in this logging for the map load status. I suggest removing it

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

…ge/ConfigureBasemapStyleLanguage.h Co-authored-by: Lucas Danzinger <[email protected]>

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: Lucas Danzinger <[email protected]>

JamesMBallard · 2024-03-05T18:18:33Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+
+ConfigureBasemapStyleLanguage::ConfigureBasemapStyleLanguage(QObject* parent /* = nullptr */) :
+  QObject(parent),
+  m_map(new Map(SpatialReference::webMercator())),


memory leak

Suggested change

m_map(new Map(SpatialReference::webMercator())),

m_map(new Map(SpatialReference::webMercator(), this)),

Also, does the map initially launch empty until users press a button? That doesn't seem optimal.

No, the map initially launches with local language strategy

JamesMBallard · 2024-03-05T18:18:56Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+// Set the view (created in QML)
+void ConfigureBasemapStyleLanguage::setMapView(MapQuickView* mapView)
+{
+  m_map = new Map(SpatialReference::webMercator());


This is already done in the constructor.

JamesMBallard · 2024-03-05T18:25:19Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.cpp

+  if (m_basemap)
+    delete m_basemap;


Suggested change

if (m_basemap)

delete m_basemap;

if (m_basemap)

m_basemap->deleteLater();

I think your code works fine, but conceptually it feels wrong to delete the map's basemap while it's still displayed. We could write a bunch of extra code to change the order, but deleteLater does the same thing with less code.

ldanzinger

LGTM. Looks like the design is approved and about to be merged. I think we might want to wait until it is officially merged before we merge this. You can watch the #native-samples-announcements channel for this

JamesMBallard · 2024-03-05T19:57:18Z

...RuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLanguage/ConfigureBasemapStyleLanguage.qml

+                ComboBox {
+                    id: comboBox
+                    model: ["none" , "Bulgarian", "Greek", "Turkish"]
+                    onCurrentTextChanged: model.setNewBasemapLanguage(globalButton.checked, comboBox.currentText);


Per our offline discussion, I think this should be more intentional. It seems this executes when the code first loads, but it's unclear from looking at the c++ code how any basemap was loaded. I think the code should be more intentional about how the initial basemap is being set.

When I reviewed the latest code, I assumed no basemap at all was set, because that's what is happening on the C++ side via the constructor.

har13205 added 2 commits March 1, 2024 15:58

Adding configure basemap style language sample

39988ec

Initializing basemapStyleParameters

4009a8b

har13205 requested review from ldanzinger and JamesMBallard March 2, 2024 02:00

har13205 self-assigned this Mar 2, 2024

JamesMBallard reviewed Mar 4, 2024

View reviewed changes

ldanzinger reviewed Mar 4, 2024

View reviewed changes

har13205 and others added 10 commits March 4, 2024 14:57

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

edd0551

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

71acdd2

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

479515d

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

50c5414

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

0379821

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

d9783d2

…ge/ConfigureBasemapStyleLanguage.h Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

3783d68

…ge/ConfigureBasemapStyleLanguage.qml Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

a9a465c

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

f64922c

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Changes as per PR comments

1769e6d

har13205 requested review from JamesMBallard and ldanzinger March 4, 2024 23:25

JamesMBallard reviewed Mar 5, 2024

View reviewed changes

har13205 and others added 3 commits March 4, 2024 18:30

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

8d3364e

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

41e07ec

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: James Ballard <[email protected]>

Changes as per PR comments

5d36cdf

har13205 requested a review from JamesMBallard March 5, 2024 02:42

ldanzinger requested changes Mar 5, 2024

View reviewed changes

har13205 and others added 5 commits March 5, 2024 10:00

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

9f52d51

…ge/ConfigureBasemapStyleLanguage.h Co-authored-by: Lucas Danzinger <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

0b36d5b

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: Lucas Danzinger <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

cd68518

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: Lucas Danzinger <[email protected]>

Update ArcGISRuntimeSDKQt_CppSamples/Maps/ConfigureBasemapStyleLangua…

c4d12b1

…ge/ConfigureBasemapStyleLanguage.cpp Co-authored-by: Lucas Danzinger <[email protected]>

Changes as per PR comments

0860c3d

har13205 requested a review from ldanzinger March 5, 2024 18:16

JamesMBallard reviewed Mar 5, 2024

View reviewed changes

Changes as per PR comments

7a6a3f5

har13205 requested a review from JamesMBallard March 5, 2024 18:41

ldanzinger approved these changes Mar 5, 2024

View reviewed changes

JamesMBallard reviewed Mar 5, 2024

View reviewed changes

Adding comment for onCurrentTextChanged

dbc50f4

har13205 requested a review from JamesMBallard March 5, 2024 21:12

JamesMBallard approved these changes Mar 5, 2024

View reviewed changes

har13205 merged commit 501830a into v.next Mar 6, 2024
1 check passed

har13205 deleted the har13205/configureBasemapStyleLanguage branch March 6, 2024 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Configure basemap style language sample #1688

Adding Configure basemap style language sample #1688

har13205 commented Mar 2, 2024

JamesMBallard Mar 4, 2024

JamesMBallard Mar 4, 2024

JamesMBallard Mar 4, 2024

ldanzinger Mar 4, 2024

JamesMBallard left a comment

JamesMBallard Mar 5, 2024

ldanzinger left a comment

ldanzinger Mar 5, 2024

JamesMBallard Mar 5, 2024

har13205 Mar 5, 2024

JamesMBallard Mar 5, 2024

JamesMBallard Mar 5, 2024

ldanzinger left a comment

JamesMBallard Mar 5, 2024

		function setNewBasemap() {
		model.setNewBasemap(globalButton.checked , comboBox.currentText);

	m_map(new Map(SpatialReference::webMercator())),
	m_map(new Map(SpatialReference::webMercator(), this)),

Adding Configure basemap style language sample #1688

Adding Configure basemap style language sample #1688

Conversation

har13205 commented Mar 2, 2024

Description

Type of change

Checklist

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

JamesMBallard left a 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

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

Choose a reason for hiding this comment