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

Upload dialog enhancements #205

Merged

Conversation

theduckylittle
Copy link
Member

  1. Normalized the CSS for a progress spinner into the 'spinner'
    class name.
  2. Spinner is now used by the service manager tab for loading.
  3. Upload dialog now tracks its own progress. It will also show
    the number of features uploaded to the layer when finished.
  4. Errors are no longer silent. But it is a pretty standard
    "something went wrong" error message.

@klassenjs
Copy link
Member

Overall it appears to work. I tested large upload success, large upload failure, service tab waiting for results.

I noticed that things got weird after uploading the parcels as GeoJSON but forgetting to transform them to 4326 first. It uploads, says success with 4040 features, but doesn't show them on the map. Trying again with the GeoJSON in 4326 again shows success with 4040 features but nothing added to the map. Clearing all features (trash can icon) and loading the correct (4326) GeoJSON works. I would suggest this error be flagged somehow as right now it just appears that upload is silently broken.

I noticed the service tab spinner spins and the upload dialog spinner doesn't spin.

@klassenjs
Copy link
Member

Offending file created with:
ogr2ogr -f GeoJSON parcels-utm.geojson . parcels

Working file created with:
ogr2ogr -f GeoJSON parcels.geojson . -t_srs EPSG:4326 parcels

@theduckylittle
Copy link
Member Author

@klassenjs - I think the projection issues can be solved by upgrading openlayers and using a newer version's projection parsing capabilities (OL can support the GeoJSON CRS format) but that would be a different PR IMO.

1. Normalized the CSS for a progress spinner into the 'spinner'
   class name.
2. Spinner is now used by the service manager tab for loading.
3. Upload dialog now tracks its own progress. It will also show
   the number of features uploaded to the layer when finished.
4. Errors are no longer silent. But it is a pretty standard
   "something went wrong" error message.
@theduckylittle theduckylittle force-pushed the feature/upload-dialog-enhancements branch from 54b46cf to 7a6f9f3 Compare August 22, 2017 12:43
@klassenjs
Copy link
Member

@theduckylittle That's really cool and we should support projections if it is easy enough, but it doesn't solve that uploading GeoJSON with a misidentified projection has the user appearance of breaking GeoMoose without even issuing a warning.

As this PR is about adding user feedback about success/failure to load features that seems like it may be relevant as another failure case to check for and notify the user.

Up to you though. GeoMoose is certainly better with this PR than without, but I wouldn't consider this a complete answer to the underlying issue (user notifications) and which may or may not have been created as a GitHub issue.

@theduckylittle
Copy link
Member Author

The biggest problem with going down the "is this good data or not" rabbit hole is that we need to make assumptions and/or sanity checks that may not be viable. The reason the feature additions appear to work is because as far as the software is concerned ... it likely did. But because the CRS was wrong it put the features on the map in a position (usually around null island) where they cannot be viewed or are scaled so small they cannot be seen.

But if we filter those out we could be forcing users into losing data that IS valid even if it is somewhat silly.

@klassenjs
Copy link
Member

It is worse than just sending some features to null island though. Afterwards you can't even load valid features and have them appear without first clearing the layer. (I haven't checked what it does to existing features. Kind of curious if they go away.)

@theduckylittle
Copy link
Member Author

It should only ADD features. There is no clear() call before the features are added.

@klassenjs
Copy link
Member

I agree that it both should do that and that it does do that.

Maybe I am not describing it well enough. Please try it. If you load parcels-utm.json, it says success in the dialog, but in reality the layer is broken until the user clears it or GeoMoose is reloaded. IMO this is bad.

@klassenjs
Copy link
Member

To be clear on the clear() call: I agree there should not be, nor is there currently a clear call in the upload code path. And I don't know why that was brought up.

@theduckylittle
Copy link
Member Author

@klassenjs - You asked how pre-existing features are handled ... I was clarifying that new features should only be added as there is no call to clear() on the source (or equivalent action dispatched).

@klassenjs
Copy link
Member

I was asking how pre-existing features are handled in the case of the bug. I was curious if the layer is going totally AWOL or just the uploader.

@klassenjs
Copy link
Member

Back to the core of the issue: It is possible (and IMO relatively easy to accidentally) upload a GeoJSON file that is both shown as successfully loaded and leaves the layer in a bad state (until manually cleared). I was able to reproduce this condition 100% of the time following the procedure I listed in my first two comments. My suggestion is we ideally fix or at least alert the user to that condition (I don't know if that would best be done in the uploader or in the layer itself).

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.

2 participants