New sample: Edit features with feature-linked annotation

[sclaridge]

Hello,

Some areas I would like some feedback are:

• the identifyFeature() method identifies a feature in the topmost layer. This implementation handles the case where the user moves a point very close to a line, then wants to move either feature later. Otherwise, an exception is thrown when both features are selected, then moved.

• I created updateAtttributes() to update the feature when it is moved, but the logic to update the user entered attributes is located within EditAttributesDialog

• in the samples review, I received feedback that the sample does not need to prompt the user if they want to move the point, as that is outlined in the README.

• is README Java specific enough?

Thanks!

[colinanderson]

Do you need to check isDone? I think just calling get() will either work or throw an exception.

[JonLavi]

Nice work @sclaridge .
I think this looks good now. There's a couple of smaller things to fix and a few suggestions on documentation, but other than that it's nearly finished. I'll approve so that you can move on to get a second review once you've done the changes 👍

[JonLavi]

Nice work on tidying this up.
I've had a closer look and there's actually no need to return the selected feature here. This is because there's a handle on it in the main sample code already (from when we run mapView.identifyLayersAsync). So for the sake of simplicty let's convert back to return null;

[JonLavi]

We can be a bit more generous here and give a larger tolerance. That way it'll be a bit easier to select the lines.

Suggested change

	ListenableFuture<List<IdentifyLayerResult>> identifyLayerResultsFuture = mapView.identifyLayersAsync(screenPoint, 1, false);
	ListenableFuture<List<IdentifyLayerResult>> identifyLayerResultsFuture = mapView.identifyLayersAsync(screenPoint, 10, false);

[sclaridge]

Increasing the tolerance forces you to zoom in to select a feature, as annotation layers are identified as the top most layer. A for loop can be added to ignore the annotation layers and select the feature. I think this implementation is cleaner, and is similar to the identify workflow in the Android sample.

// identify across all layers
ListenableFuture<List<IdentifyLayerResult>> identifyLayerResultsFuture = mapView.identifyLayersAsync(screenPoint, 10, false);
    identifyLayerResultsFuture.addDoneListener(() -> {
      try {
        // get the list of results from the future
        List<IdentifyLayerResult> identifyLayerResults = identifyLayerResultsFuture.get();
        // for each layer from which an element was identified
        for (IdentifyLayerResult layerResult : identifyLayerResults) {
          // check if the layer is a feature layer, thereby excluding annotation layers
          if (layerResult.getLayerContent() instanceof FeatureLayer) {
            // get a reference to the identified feature
            selectedFeature = (Feature) layerResult.getElements().get(0);
            // check the geometry and select the feature
            selectFeature(layerResult);
            return;
          }
     }

[JonLavi]

I personally think we can simplify this by turning the if-statment around. Doing an 'early return' is very much a C# pattern, as far as I'm aware.

Suggested change

	// if the selected feature is a polyline with any part containing more than one segment
	// (i.e. a curve)
	if (part.getPointCount() > 2) {
	selectedFeature = null;
	// show message reminding user to select straight (single segment) polylines only
	new Alert(Alert.AlertType.WARNING, "Select straight (single segment) polylines only.").show();
	// return early, effectively disallowing selection of multi segmented polylines
	return;
	} else {
	// select the polyline feature
	((FeatureLayer) layerResult.getLayerContent()).selectFeature(selectedFeature);
	selectedFeatureIsPolyline = true;
	}
	// only select single segment lines
	if (part.getPointCount() <= 2) {
	// select the polyline feature
	((FeatureLayer) layerResult.getLayerContent()).selectFeature(selectedFeature);
	selectedFeatureIsPolyline = true;
	} else {
	selectedFeature = null;
	// show message reminding user to select straight (single segment) polylines only
	new Alert(Alert.AlertType.WARNING, "Select straight (single segment) polylines only.").show();
	}

[JonLavi]

It doesn't look like there's any need for this try/catch block anymore, since all the asynchronous actions are now being done in other methods? In which case we can just remove it

[Rachael-E]

@sclaridge this is a really great effort on this rather complex sample!

My comments here so far are more from a readability/code tidiness perspective and I think there's some more we can do here to neaten up this sample. Let's have a pair programming call and I can talk you through them.

To discuss:

• Java SE samples and single use methods
• edge case user input control
• sample bug when moving polylines (it looks like additional polylines are being created, rather than just an individual line being moved)
• samples-data directory and some background there for the settings.gradle and build.gradle file (including removing the actual geodatabase from this repo)
• garbage collection

[Rachael-E]

this description doesn't include actually moving an address point on the sample. This threw me initially as I clicked "OK" or "Cancel" and then clicked on the map/another point: wasn't expecting the address point to move too. We should add in some additional documentation for this:

Suggested change

Pan and zoom the map to see that the text on the map is annotation, not labels. Click one of the address points to update the house number (AD_ADDRESS) and street name (ST_STR_NAM). Click one of the dashed parcel polylines and click another location to change its geometry. NOTE: Selection is only enabled for points and straight (single segment) polylines.

Pan and zoom the map to see that the text on the map is annotation, not labels. Click one of the address points to update the house number (AD_ADDRESS) and street name (ST_STR_NAM). Once you have edited the feature attributes, click "OK" and then click again on the map to move the address point to a new location. You can also click one of the dashed parcel polylines and click another location to change its geometry and update its annotation (distance in feet). NOTE: Selection is only enabled for points and straight (single segment) polylines.

[Rachael-E]

I added a comment in Jon's review, but just in case it gets lost: samples data should always be saved into a samples-data directory. The resources are reserved for UI or style files.

Suggested change

	dest: file("./src/main/resources/edit_features_with_feature_linked_annotation/loudon")
	dest: file("./samples-data/loudon")

[Rachael-E]

by the time we're using local variables for the FeatureLayer's here (see comment above), that's a lot of references to the word FeatureLayer in this block of code. For situations like this, we can make use of Java 10's keyword "var". This replaces the type information when declaring local variables and clears the readability up a little.

Also since these are quite long lines of code, we should stagger them a little:

Suggested change

	// create feature layers from tables in the geodatabase
	addressPointFeatureLayer = new FeatureLayer(geodatabase.getGeodatabaseFeatureTable("Loudoun_Address_Points_1"));
	parcelLinesFeatureLayer = new FeatureLayer(geodatabase.getGeodatabaseFeatureTable("ParcelLines_1"));
	// create annotation layers from tables in the geodatabase
	addressPointsAnnotationLayer = new AnnotationLayer(geodatabase.getGeodatabaseAnnotationTable("Loudoun_Address_PointsAnno_1"));
	parcelLinesAnnotationLayer = new AnnotationLayer(geodatabase.getGeodatabaseAnnotationTable("ParcelLinesAnno_1"));
	// create feature layers from tables in the geodatabase
	var addressPointFeatureLayer =
	new FeatureLayer(geodatabase.getGeodatabaseFeatureTable("Loudoun_Address_Points_1"));
	var parcelLinesFeatureLayer =
	new FeatureLayer(geodatabase.getGeodatabaseFeatureTable("ParcelLines_1"));
	// create annotation layers from tables in the geodatabase
	var addressPointsAnnotationLayer =
	new AnnotationLayer(geodatabase.getGeodatabaseAnnotationTable("Loudoun_Address_PointsAnno_1"));
	var parcelLinesAnnotationLayer =
	new AnnotationLayer(geodatabase.getGeodatabaseAnnotationTable("ParcelLinesAnno_1"));

[Rachael-E]

I think for a readability perspective, it makes more sense for us to move this loadAsync call up (to above the geodatabase.addDoneLoadingListener() even though it doesn't make much of a difference from a code execution perspective.

[Rachael-E]

with moving the logic from the single use method directly into this listener, we can borrow some of the original method description and use it here instead:

Suggested change

	// set on click behaviour
	// select the nearest feature from where the user clicked, or move the selected feature to the given screen point

[Rachael-E]

we can remove this comment: the code below it is clear enough to describe it is checking the primary mouse button was clicked.

Suggested change

// check that the primary mouse button was clicked

[Rachael-E]

it's common with the Android implementations to have single use methods, but with Java SE samples we don't do this as often. The logic in this method isn't too verbose, so let's just remove the method and insert the code from that method directly here instead.

Suggested change

	selectOrMove(screenPoint);
	// if a feature hasn't been selected, select the feature
	if (selectedFeature == null) {
	identifyFeature(screenPoint);
	} else {
	// convert the screen point to a map point
	Point mapPoint = mapView.screenToLocation(screenPoint);
	// if the feature is a polyline, move the polyline
	if (selectedFeatureIsPolyline) {
	movePolylineVertex(mapPoint);
	} else {
	// if the feature is a point, move the point
	movePoint(mapPoint);
	}
	}```

[Rachael-E]

rather than casting this long to an int (which could result in a stack overflow if the long is greater than the maximum allowed int value), we can use the following method which would throw an informative exception in case of the above happening. Very unlikely in this sample but good practice I believe.

Suggested change

	Part part = polylineBuilder.getParts().get((int) nearestVertex.getPartIndex());
	Part part = polylineBuilder.getParts().get(Math.toIntExact(nearestVertex.getPartIndex()));

[Rachael-E]

since we don't use the variable parts more than once, we can directly add the forEach like so:

Suggested change

	List<Part> parts = polylineBuilder.getParts();
	parts.forEach(part -> {
	polylineBuilder.getParts().forEach(part -> {

[Rachael-E]

@sclaridge this is looking great. I've suggested just a few really minor comment updates.

Before merging, please can you ensure the rogue geodatabase is deleted from this PR? https://github.com/Esri/arcgis-runtime-samples-java/blob/924c240f55b27603be178aef26019340df599dc7/editing/edit-features-with-feature-linked-annotation/src/main/resources/edit_features_with_feature_linked_annotation/loudon/loudoun_anno.geodatabase

Once you've done this I'm happy for you to go ahead and merge. Good job!

[Rachael-E]

just some wordsmithing here:

Suggested change

	// add the feature layers and annotation layers to the map
	// add the feature and annotation layers to the map

[Rachael-E]

correcting apostrophe

Suggested change

	// set the selected features' geometry to a new map point
	// set the selected feature's geometry to a new map point

[Rachael-E]

just a suggestion: I think this might be a littler clearer, but up to you if you want to accept this change!

Suggested change

	// clear the selected feature
	// remove the selection of the feature