-
Notifications
You must be signed in to change notification settings - Fork 123
Fclfp hardik #463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fclfp hardik #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @hardikc07! Well done on doing the full MVC pattern on this sample. It certainly is a good exercise. However I think in this sample we can make the UI in the main file, and don't need a Controller
file and fxml
, since it's a very small amount of UI (1 Text field and 1 button). Other than that, the code looks quite good. If you could restructure the sample to only be in one file, I'll have another closer look. Also, don't forget to update the README.metadata.json file. It's not the most exciting bit of work, but we definitely need it for the Developer Page to work correctly.
feature_layers/feature-collection-layer-from-portal/README.metadata.json
Outdated
Show resolved
Hide resolved
feature_layers/feature-collection-layer-from-portal/README.metadata.json
Outdated
Show resolved
Hide resolved
|
||
// load the portal and add the portal item | ||
Portal portal = new Portal("https://www.arcgis.com/"); | ||
System.out.println(FeatureCollectionItemIdTextField.getText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to get rid of all the console logging when you're finished 😉 Happens to me too, so no worries!
// create feature collection and add to the map as a layer | ||
FeatureCollection featureCollection = new FeatureCollection(portalItem); | ||
FeatureCollectionLayer featureCollectionLayer = new FeatureCollectionLayer(featureCollection); | ||
map.getOperationalLayers().add(featureCollectionLayer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the pattern we try and promote is to only add the layers to the map's operational layers after we've checked that the appropriate resource has loaded successfuly.
So in this case I'd suggest putting this block of code where you create the FeatureCollection and Layer from the Portal inside the if (portalItem.getLoadStatus() == LoadStatus.LOADED) {
statement on line 80
@@ -0,0 +1,64 @@ | |||
/* | |||
* Copyright 2016 Esri. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to update the copyright year 😉
* Copyright 2016 Esri. | |
* Copyright 2020 Esri. |
@Override | ||
public void start(Stage stage) throws IOException { | ||
// set up the scene | ||
FXMLLoader loader = new FXMLLoader(getClass().getResource("/fclfp.fxml")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to keep the MVC structure here, but just pointing out that it would be best to name the fxml
file with a more clear file name (generally the samples name with underscores), so feature_collection_layer_from_portal.fxml
@@ -0,0 +1,27 @@ | |||
.panel-region .label { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going forward we're trying to limit this CSS file to only the rules that are being used. Once you've changed the sample to only only live in one file, just check which of these you are using, and delete the rest.
Co-Authored-By: Jonathan Lavi <[email protected]>
Co-Authored-By: Jonathan Lavi <[email protected]>
Co-Authored-By: Jonathan Lavi <[email protected]>
removed controller file , added code into one java file and also removed fxml/css file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @hardikc07 ! It's looking a lot more concise now. There's just a bit of rearranging of the code I would suggest, so that it's easier for users to follow the main workflow. I've also found a few places where we can make the comment style more consistent. Give me a shout if you have any questions!
feature_layers/feature-collection-layer-from-portal/README.metadata.json
Outdated
Show resolved
Hide resolved
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Outdated
Show resolved
Hide resolved
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Show resolved
Hide resolved
feature_layers/feature-collection-layer-from-portal/README.metadata.json
Outdated
Show resolved
Hide resolved
featureCollection = new FeatureCollection(portalItem); | ||
featureCollectionLayer = new FeatureCollectionLayer(featureCollection); | ||
|
||
featureCollectionLayer.loadAsync(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add the line map.getOperationalLayers().add(featureCollectionLayer);
above, it would automatically trigger loading the layer, so you can get rid of the 'loadAsync();call here. I think generally the trick with loadables is that if you're adding them to the map or performing some sort of operation with them, they will start loading by themselves. If you just instantiate them and do nothing else, you'd need the
loadAsync()`.
if (!"".equals(input.getText())) { | ||
fetchFeatureCollectionFromPortal(); | ||
} else { | ||
new Alert(Alert.AlertType.ERROR, "Portal Itemid is empty. Please enter a portal item id.").show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this check for the contents of the inputTextBox
directly in the method fetchFeatureCollectionFromPortal()
. That way all you have to do is:
fetchFromPortal.setOnAction(e -> fetchFeatureCollectionFromPortal());
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Show resolved
Hide resolved
|
||
//create portal and portal item | ||
portal = new Portal("https://www.arcgis.com/"); | ||
PortalItem portalItem = new PortalItem(portal, input.getText()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do the check for the contents of the portal item directly after this, then continue creating the FCL. At the end of the block you can then show an error if it's not a valid item:
if (portalItem.getType() == PortalItem.Type.FEATURE_COLLECTION) {
// create a feature collection from the portal item
// create a feature collection layer
// add the feature collection layer to the map
else {
// show the error
}
@@ -1,5 +1,6 @@ | |||
#Wed Mar 25 17:43:43 PDT 2020 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to keep this comment in here.
making changes as per review by jonathan Co-Authored-By: Jonathan Lavi <[email protected]>
"id", | ||
"item", | ||
"map", | ||
"notes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing "
which is why GitHub is showing this as invalid JSON.
"notes, | |
"notes", |
stage.show(); | ||
|
||
|
||
// create a new ArcGISMap with an oceans Basemap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// create a new ArcGISMap with an oceans Basemap | |
// create a new ArcGISMap with the oceans basemap |
// create a new ArcGISMap with an oceans Basemap | ||
map = new ArcGISMap(Basemap.createOceans()); | ||
|
||
// create a map view and set the ArcGISMap to it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// create a map view and set the ArcGISMap to it | |
// create a map view and set the map to it |
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Outdated
Show resolved
Hide resolved
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Outdated
Show resolved
Hide resolved
...sri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes needed. I agree with Jon's feedback too. Good work so far!
made changes as per suggestion in pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is almost ready @hardikc07, nice work! I'm just suggesting a bit of reordering to the method that creates the FCL, so that it's easier to follow for users. Let me know what you think!
@@ -16,6 +16,7 @@ | |||
|
|||
package com.esri.samples.feature_collection_layer_from_portal; | |||
|
|||
import com.esri.arcgisruntime.loadable.LoadStatus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make sure to put this import in the right order (with the bloc below).
I can really recommend you set up IntelliJ to order the imports for you, so you won't ever have to worry about it again.
// create portal | ||
portal = new Portal("https://www.arcgis.com/"); | ||
|
||
// create a map view and set the map to it | ||
mapView = new MapView(); | ||
mapView.setMap(map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest swapping the order of these two lines. That way we deal with the map and map view first, and then create the portal. It should be a bit easier to follow.
// create portal | |
portal = new Portal("https://www.arcgis.com/"); | |
// create a map view and set the map to it | |
mapView = new MapView(); | |
mapView.setMap(map); | |
// create a map view and set the map to it | |
mapView = new MapView(); | |
mapView.setMap(map); | |
// create portal | |
portal = new Portal("https://www.arcgis.com/"); |
inputTextField = new TextField(); | ||
inputTextField.setMaxWidth(250); | ||
inputTextField.setText("32798dfad17942858d5eef82ee802f0b"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do as Tyler suggested and put the contents of the text field in the constructor:
inputTextField = new TextField(); | |
inputTextField.setMaxWidth(250); | |
inputTextField.setText("32798dfad17942858d5eef82ee802f0b"); | |
inputTextField = new TextField("32798dfad17942858d5eef82ee802f0b"); | |
inputTextField.setMaxWidth(250); |
}); | ||
inputTextField = new TextField(); | ||
inputTextField.setMaxWidth(250); | ||
inputTextField.setText("32798dfad17942858d5eef82ee802f0b"); | ||
|
||
fetchFeatureCollectionFromPortal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a small comment here to help the users see what's going on:
fetchFeatureCollectionFromPortal(); | |
// fetch the default feature collection layer from the portal | |
fetchFeatureCollectionFromPortal(); |
/** | ||
* Action event on button and validation of empty Text Filed | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment still appears in JavaDoc style for me. Could you just double check it's in a single line?
else | ||
{ | ||
new Alert(Alert.AlertType.ERROR, "Portal Item ID is empty. Please enter a Portal Item ID.").show(); | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this! There's only a couple of things left to tidy here:
A bit of whitespace would help make the users read this a little easier. As a general rule of thumb, we try and add a blank line between each block of code.
For the sake of making an easy to follow sample, we'd want to do a few things and consider the order of doing them:
- check that the portal item ID is provided
- check that we can load the portal item, and that it loaded successfuly (Adding the FCL should only happen after we check that the portal item has done loading successfuly. That way we don't perform the action before the appropriate resource is loaded. )
- check that the portal item is a feature collection (We need to check that the portal item is a feature collection before actually trying to use it to create the FCL )
- create the feature collection layer
- add the later to the map.
This would make it a lot easier for users to read the sample and understand in which order stuff needs to be done.
Keep in mind that we don't generally print out the loading error using portalItem.getLoadError().getCause().toString()
, as you've done, because it will be displayed to they end-user, and they wouldn't know what to do with the technical information. Instead we just write a message that describes what went wrong.
} | |
/** | |
* Fetch a feature collection from a portal and load it as a feature layer. | |
*/ | |
private void fetchFeatureCollectionFromPortal() { | |
if (!inputTextField.getText().isEmpty()) { | |
// crate portal item | |
PortalItem portalItem = new PortalItem(portal, inputTextField.getText()); | |
// load the portal item | |
portalItem.loadAsync(); | |
portalItem.addDoneLoadingListener(() -> { | |
if (portalItem.getLoadStatus() == LoadStatus.LOADED) { | |
// check that the portal item is a feature collection | |
if (portalItem.getType() == PortalItem.Type.FEATURE_COLLECTION) { | |
// create a feature collection layer | |
featureCollection = new FeatureCollection(portalItem); | |
FeatureCollectionLayer featureCollectionLayer = new FeatureCollectionLayer(featureCollection); | |
// add the feature layer to the map | |
map.getOperationalLayers().clear(); | |
map.getOperationalLayers().add(featureCollectionLayer); | |
} else { | |
new Alert(Alert.AlertType.ERROR, "Portal Item is not a valid Feature Collection.").show(); | |
} | |
} else { | |
new Alert(Alert.AlertType.ERROR, "Failed to load Portal Item.").show(); | |
} | |
}); | |
} else { | |
new Alert(Alert.AlertType.ERROR, "Portal Item ID is empty. Please enter a Portal Item ID.").show(); | |
} | |
} | |
few more changes as per review by jonathan - removed javadoc comment - set text in costructor - import order / map portal oder fixes - validation of portalitem and loading status before creating FC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made changes as suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @hardikc07 !
Only found a typo and a little bit of formatting to fix, but after that I think it's good.
|
||
// fetch the default feature collection layer from the portal | ||
fetchFeatureCollectionFromPortal(); | ||
|
||
// create button to perform action | ||
Button fetchFromProtalButton = new Button("Open from portal item"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the name of the button:
fetchFromPortalButton
} | ||
}); | ||
|
||
} else | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the newline here between else
and {
} | ||
portalItem.loadAsync(); | ||
|
||
portalItem.addDoneLoadingListener(()->{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bit of formatting here:
portalItem.addDoneLoadingListener(()->{ | |
portalItem.addDoneLoadingListener(() -> { |
- typo in button name - remove extra sapce for else cond - added space in listener method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more small things to change. Please mark the feedback as Resolved when you make the updates.
3. Verify that the item represents a feature collection. | ||
4. Create a `FeatureCollection` from the item. | ||
5. Create a `FeatureCollectionLayer`, referring to the feature collection. | ||
6. Add the feature collection layer to the map's `OperationalLayers` collection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6. Add the feature collection layer to the map's `OperationalLayers` collection. | |
6. Add the feature collection layer to the map's operational layers collection. |
"snippets": [ | ||
"src/main/java/com/esri/samples/feature_collection_layer_from_portal/FeatureCollectionLayerFromPortalSample.java" | ||
], | ||
"title": "Feature Collection Layer From Portal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"title": "Feature Collection Layer From Portal" | |
"title": "Feature Collection Layer from Portal" |
new Alert(Alert.AlertType.ERROR, "This is not valid Feature Collection.").show(); | ||
} | ||
} else { | ||
new Alert(Alert.AlertType.ERROR, "Item does not exist or is inaccessible").show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Alert(Alert.AlertType.ERROR, "Item does not exist or is inaccessible").show(); | |
new Alert(Alert.AlertType.ERROR, "Portal item failed to load").show(); |
map.getOperationalLayers().add(featureCollectionLayer); | ||
|
||
} else { | ||
new Alert(Alert.AlertType.ERROR, "This is not valid Feature Collection.").show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Alert(Alert.AlertType.ERROR, "This is not valid Feature Collection.").show(); | |
new Alert(Alert.AlertType.ERROR, "Portal item is not a feature collection").show(); |
private MapView mapView; | ||
private ArcGISMap map; | ||
private Portal portal; | ||
private FeatureCollection featureCollection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a local variable in the fetch method.
}); | ||
} else | ||
{ | ||
new Alert(Alert.AlertType.ERROR, "Portal Item ID is empty. Please enter a Portal Item ID.").show(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Alert(Alert.AlertType.ERROR, "Portal Item ID is empty. Please enter a Portal Item ID.").show(); | |
new Alert(Alert.AlertType.ERROR, "Portal Item ID is empty").show(); |
- alert message - readme
No description provided.