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

FileProvider #705

Closed
wants to merge 7 commits into from
Closed

FileProvider #705

wants to merge 7 commits into from

Conversation

mbredif
Copy link
Collaborator

@mbredif mbredif commented Mar 27, 2018

This PR falls under the umbrella of #695 of refactoring providers, parsers and symbolizers.
It is based on the parser registry and will be rebased once #723 is merged.

  • the new FileProvider provides a new file layer :
    • it takes an input File.
    • it leverages the parser registry to figure out the suitable parser, providing an option.crs parameter for the desired crs.
    • the result of the parsing is a dictionary
    • if this dictionary has no object3d key, then it uses the layer.convert mechanism (cf WFSProvider) to get one.
    • add the object3d to the layer (with code to get picking right)

This FileProvider enables the drag'n'drop use case #702 to be implemented rather simply as a new example.

Version 2

This PR falls under the umbrella of #695 of refactoring providers, parsers and symbolizers.

  • As a next step to refactor(parsers): move and rename parsers to a new parser directory #696, this PR adds a Parser registry to the scheduler (similar to the protocol registry), so as to decouple protocols from formats (here and here)
  • the new FileProvider provides a new file layer :
    • it takes an input File.
    • it leverages the parser registry to figure out the suitable parser, providing an option.crs parameter for the desired crs.
    • the result of the parsing is a dictionary
    • if this dictionary has no object3d key, then it uses the layer.convert mechanism (cf WFSProvider) to get one.
    • add the object3d to the layer (with code to get picking right)

To make this work on GPX and GeoJSON->Mesh, I normalized the parser api :

  • to return a dictionary which is a feature (see GeojsonParser output) or has a object3d key (GpxParser returned the THREE.Object3D directly).
  • the crs of the parsed dataset is provided as a crs option key (was crsOut in GeojsonParser)

This FileProvider enables the drag'n'drop use case #702 to be implemented rather simply as a new example.

Version 1

[WIP] preliminary file drop support

work in progress for #702

  • adds a format registry (similar to the protocol registry) to the scheduler
  • adds the drop event listener to the view

with this PR, you can take any example and drop a gpx file.

@mbredif mbredif added the wip 🚧 Still being worked on label Mar 27, 2018
@mbredif mbredif assigned gchoqueux and unassigned gchoqueux Mar 27, 2018
Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early feedback:

I'm pretty against the change in View.js, as it is the role of application developer to build their own dnd logic.

That being said, as we are splitting providers in providers + parsers, I think it's a good idea to have a File provider. This provider would have more or less what is in the current drop listener in its preprocessDataLayer method, I guess.

This would let the user deals with their File objects as they see fit, so be more flexible: you can get File instances by other means that drag&drop and we would support this too.

Fundamentally, we should not forget that iTowns is a framework not an app. We must help user build their own app, but let them make their own choices.

src/Core/View.js Outdated
const prevDefault = e => e.preventDefault();
viewerDiv.addEventListener('dragenter', prevDefault, false);
viewerDiv.addEventListener('dragover', prevDefault, false);
viewerDiv.addEventListener('dragleave', prevDefault, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember that iTowns is a framework, not an app. It is made to build other app that can have vastly different requirements. So you cannot call preventDefault on these events by default.

I even think a drop listener has nothing to do in iTowns. The way user gets files is implementation dependant. Also, they might use a dnd lib for instance.

@mbredif
Copy link
Collaborator Author

mbredif commented Mar 30, 2018

I agree that it should not be hardcoded this way in the view.
On the contrary, I think we should provide an easy way to configure drag n drop, unless we want every user to reimplement this functionality over and over again.

@autra
Copy link
Contributor

autra commented Mar 30, 2018

I think we should provide an easy way to configure drag n drop, unless we want every user to reimplement this functionality over and over again.

What if they don't want drag & drop? What if they want to use draggable.js or interact.js (because they already use it in their application elsewhere)? What if they want to do something completely different with drag&drop? What if?

You need to shift your focus from the (very nice, but all more or less the same) demos we have seen during the code sprint to real-life application, integrated into bigger websites, with vastly different use cases and requirements. Therefore, we need to start from the common point.

@mbredif
Copy link
Collaborator Author

mbredif commented Mar 30, 2018

What if they don't want drag & drop? What if they want to use draggable.js or interact.js (because they already use it in their application elsewhere)? What if they want to do something completely different with drag&drop? What if?

As I said, I agree, putting the code in View was to give a quick working draft. dnd helper code should definitely be opt in and go in a separate file (not even in the bundle).

My personal use case is not production of big websites, it is demos, proofs of concept, research and education. So ease of use is paramount. I do not know anything about draggalbe.js or interact.js and I do not want to!. I do not want any of my students either (unless they really want/have to) !

That being said, taking your concerns into account, I have just updated the PR to use a new FileProvider and move the addEventListeners to the examples/filedrop.html .

@mbredif mbredif force-pushed the filedrop branch 3 times, most recently from 837276e to 7e6b871 Compare April 3, 2018 16:20
@mbredif mbredif changed the title [WIP] preliminary file drop support Format registry and FileProvider Apr 4, 2018
@mbredif mbredif added ready and removed wip 🚧 Still being worked on labels Apr 4, 2018
@mbredif
Copy link
Collaborator Author

mbredif commented Apr 4, 2018

@autra what do you think of it now ?
drag and drop logic is now isolated in an example and the FileProvider is now fairly generic.
The example should work at least on GPX and geojson files.

@mbredif mbredif requested review from peppsac and gchoqueux April 9, 2018 16:28
@zarov
Copy link
Contributor

zarov commented Apr 11, 2018

Very cool to do this ! It is really enjoyable to drop files like this, it has potential for a more complete example, with 3D object positioning and such !

Could you be more detailed in the example (in term of UI) on what to do and which type of file is supported ?

@autra
Copy link
Contributor

autra commented Apr 11, 2018

@mbredif could you extract the file registry part to its own PR please? It needs to be worked on in itself, as there is a bit more work associated with it to be done. Thanks!

@mbredif
Copy link
Collaborator Author

mbredif commented Apr 11, 2018

This PR may be split into 5 atomic parts (with no file overlap) :

  1. format registry
  • src/Core/Scheduler/Scheduler.js
  1. file provider (depends on 1)
  • src/Provider/FileProvider.js
  1. provide an example (depends on 2) (that happen to be a drag and drop, but could have been for instance a file upload form)
  • examples/filedrop.html
  1. normalize GpxParser output to {object3d} (depends on 3, make it work for gpx files)
  • examples/gpx.html
  • src/Parser/GpxParser.js
  1. normalize GeoJsonParser input option to crs (instead of crsOut) (depends on 3, make it work for geojson files)
  • src/Parser/GeoJsonParser.js
  • src/Provider/RasterProvider.js
  • test/feature2mesh_unit_test.js
  • test/featureUtils_unit_test.js

To actually do something, you need 1234 or 1235. What do you have in mind?
#723 is a PR with 1 alone that does not really do anything.

@zarov
Copy link
Contributor

zarov commented Apr 11, 2018

Imho you should move all the things except the file drop example into the new PR (that would be 1245)

@mbredif
Copy link
Collaborator Author

mbredif commented Apr 12, 2018

#723 now addresses 1,4 and 5 among others.
I will rebase this once #723 is merged with only the 2 and 3 (fileprovider and filedrop example)

@mbredif mbredif changed the title Format registry and FileProvider FileProvider Apr 13, 2018
@mbredif
Copy link
Collaborator Author

mbredif commented Apr 13, 2018

@peppsac comment in #723

Do we even want to introduce a FileProvider?

At the last codesprint we exchanged on iTowns' API and the need of moving away from the current layer/provider design and instead implement something similar to OpenLayers or vector-tiles = splitting sources and layers.

So IMHO we should work toward this goal instead of adding new features built on the current broken layer/provider API.

We may not want exactly a FileProvider but we will definitely want something like a FileSource (or whatever name we eventually decide).

For instance, the first step to turning the hardcoded logic of the RasterizerProvider (fileurl -fetch-> geojson/gpx/kml -> geojson -> imagerylayer) into something better is IMHO to use this FileProvider (fileurl -fetch-> file -parser_registry-> parsed -convert-> layer).

@gchoqueux
Copy link
Contributor

With PR #798, your file would be represented by a source object and there would be no real need for fileProvider.

const layer = {
  id, 
  type,
  source: {  // describe your source
     protocol: 'file'
     url: './folder/file.geojson'
  }
  convert, // function to convert your file
}

@mbredif
Copy link
Collaborator Author

mbredif commented Jul 2, 2018

indeed, I'll close this when the functionality will be available.

@zarov
Copy link
Contributor

zarov commented Jan 15, 2019

We got FIleSource now for quite some time, I'm closing this PR now that #723 is also closed.

@zarov zarov closed this Jan 15, 2019
@zarov zarov deleted the filedrop branch June 18, 2019 09:13
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.

4 participants