Skip to content
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

fix(im): handle CSV import from Places tab #1192

Closed
wants to merge 1 commit into from

Conversation

bradh
Copy link
Collaborator

@bradh bradh commented Feb 24, 2021

Resolves #740 - ensures that CSV, SHP and GeoJSON are all handled by the import dialog off Places.

However I'm not sure this is the right behaviour either. Importing things on Places works, but they may not show up on Places (and don't look like placemarks or KML tracks).

If this is the desired behaviour, then it might be possible to remove the multiple import managers, and just use the global instance.

@jsalankey
Copy link
Contributor

jsalankey commented Feb 24, 2021

It is correct that we don't want to support just any kind of imports with the button on the Places window. That uses a separate import stack because we want to import any data that you pick with it directly into Places (rather than importing it as a layer and then subsequently saving that layer to Places). With KML imports in particular, we try to merge the tree into the Places tree, which is why it only supports KML currently.

The desired behavior would be to pipe the resultant features from parsing the other file types directly into a folder in the Places layer. We'd likely still want things like CSV to run through their current import UIs, but to have a special handling of the final result. This might involve extending some aspect of the import UIs, or possibly refactoring them to pass a success handler to them.

@bradh
Copy link
Collaborator Author

bradh commented Feb 24, 2021

I'm good with the first paragraph. I'm not sure I follow the second paragraph.

Are the required formats for that special handling: CSV, GeoJSON and Shapefile? Should there be a way to filter before adding as Places?

@jsalankey
Copy link
Contributor

Currently, the import UIs like plugin.file.csv.ui.CSVImportUI are registered to the global file Import Manager for the purpose of parsing out files into separate layers. When you register them with the Places Import Manager, they still create layers on success, which might be unintuitive to the user.

The import UIs registered currently by PlacesPlugin are different from the standard KML/KMZ import UIs. They are registered here: https://github.com/ngageoint/opensphere/blob/master/src/plugin/places/placesplugin.js#L77-L79.

In kmlplacesimportui.js plugin.places.KMLPlacesCtrl.prototype.onImportComplete_, the success handler doesn't create a new layer, it adds the results to the Places tree. For CSV/GeoJSON/SHP, I was envisioning either whole new import UI classes or possibly a refactor of the existing ones to allow changing how they handle successfully parsing files that add the results to Places instead of a new layer.

I'm not sure I'm following the filter question, but I don't think we'd be looking to add anything filtering related here.

@bradh bradh closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handle CSV import via Places Import
2 participants