LocalFileSource #6497

ivovandongen · 2016-09-28T14:07:36Z

mention-bot · 2016-09-28T14:07:38Z

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmpsantos and @mollymerp to be potential reviewers.

ivovandongen · 2016-09-28T15:38:32Z

@tmpsantos Could you review please (assuming the ci completes)?

I'm not very happy with the name FileFileSource, but couldn't come up with a better one.

1ec5 · 2016-09-29T05:49:36Z

I'm not very happy with the name FileFileSource, but couldn't come up with a better one.

How about LocalFileSource?

1ec5 · 2016-09-29T05:55:10Z

With these changes, is it still necessary for the default AssetFileSource to know how to handle absolute paths? Also, this iOS/macOS special case can be removed now that file: URLs are supported in core.

brunoabinader · 2016-09-29T08:36:55Z

How about LocalFileSource?

I like this one - though we should be aware that the file URI scheme can also point to files on a network location.

tmpsantos · 2016-09-29T08:48:45Z

How about LocalFileSource?

👍

ivovandongen · 2016-09-29T10:45:19Z

How about LocalFileSource?

Seems popular :) I've changed this.

is it still necessary for the default AssetFileSource to know how to handle absolute paths?

I assumed we need to leave it in there now for backwards compatibility.

this iOS/macOS special case can be removed now that file: URLs are supported in core.

Removed

tmpsantos · 2016-09-29T11:12:17Z

platform/default/default_file_source.cpp

@@ -19,6 +20,12 @@ bool isAssetURL(const std::string& url) {
 return std::equal(assetProtocol.begin(), assetProtocol.end(), url.begin());
 }

+const std::string fileProtocol = "file://";


Better avoid this. See for alternatives: #6455

tmpsantos · 2016-09-29T11:15:30Z

test/storage/local_file_source.test.cpp

+
+namespace {
+
+std::string toAbsoluteUrl(const std::string& fileName) {


Nit: we use URL on the internal bits rather then Url.

tmpsantos · 2016-09-29T11:17:41Z

test/storage/local_file_source.test.cpp

+std::string toAbsoluteUrl(const std::string& fileName) {
+ char buff[PATH_MAX + 1];
+ char* cwd = getcwd( buff, PATH_MAX + 1 );
+ return "file:://" + std::string(cwd) + "/test/fixtures/storage/assets/" + fileName;


tmpsantos · 2016-09-29T11:20:35Z

platform/default/local_file_source.cpp

+public:
+ void request(const std::string& url, FileSource::Callback callback) {
+ //Cut off the protocol
+ std::string path = mbgl::util::percentDecode(url.substr(7));


Make this like #6455 with the size a global together with the scheme variable.

tmpsantos · 2016-09-29T11:22:20Z

test/storage/local_file_source.test.cpp

+ loop.run();
+}
+
+TEST(LocalFileSource, URLEncoding) {


For the sake we are no missing anything, can you make a test with a extremely large file name, like memset(filename, 'x', PATH_MAX);

Wondering what would happen if you try to read something you don't have permission to (like /root/foo.txt).

ivovandongen · 2016-09-29T15:09:32Z

@tmpsantos Thanks for the review. I've cleared up all items.

Wondering what would happen if you try to read something you don't have permission to

It returns Response::Error::Reason::Other. Not quite sure how to test this in a platform independent way though.

tmpsantos · 2016-09-29T17:04:24Z

platform/default/local_file_source.cpp

+
+namespace mbgl {
+
+const char* protocol = "file://";


This could go to the anonymous namespace.

Thanks! Changed it.

tmpsantos · 2016-09-29T19:31:10Z

platform/default/local_file_source.cpp

+#include <sys/stat.h>
+#include <unistd.h>
+
+const char* protocol = "file://";


namespace { const char* protocol = "file://"; const std::size_t protocolLength = 7; } // namespace

1ec5 · 2016-09-29T21:18:28Z

is it still necessary for the default AssetFileSource to know how to handle absolute paths?

I assumed we need to leave it in there now for backwards compatibility.

True, removing that support might (?) break asset: assets that have been cached, I suppose, and it’s pretty harmless.

ivovandongen · 2016-09-30T10:31:46Z

@tmpsantos I fixed the valgrind errors. If all else is good, could you press the review button?

tmpsantos

👍 for [core]

tobrun · 2016-09-30T12:56:50Z

ericpalakovichcarr · 2016-09-30T13:02:17Z

I just wanted to say to @ivovandongen, thank you! Oh, and also @1ec5 and the rest of the mapbox team as well 👍

zugaldia · 2016-09-30T13:23:05Z

cc: @cammace this looks like a good candidate for a new sample, or a required update to https://www.mapbox.com/android-sdk/examples/custom-raster/.

SeanChristopherConway · 2017-01-16T22:23:34Z

I am using ios SDK 3.4.0-beta.6 and am not able to use the file:// style to load my style JSON or in the tiles section of my sources in the style.json. When I use asset:// for the style it loads, but neither asset nor file seem to work within the styles section.

1ec5 · 2017-01-16T23:32:31Z

It should be possible to use an absolute file: URL as an MGLMapView’s style URL. But I’m not sure if it’s supposed to be possible to use a file: URL inside the JSON file itself. In any event, this closed PR likely isn’t being watched by folks who know the answer for sure, so please open a new issue, ideally with more details about your style JSON.

ivovandongen added the Core The cross-platform C++ core, aka mbgl label Sep 28, 2016

ivovandongen self-assigned this Sep 28, 2016

ivovandongen force-pushed the 6273-fs-filesource branch 2 times, most recently from b997865 to a8d0eee Compare September 28, 2016 15:09

ivovandongen added this to the android-v4.2.0 milestone Sep 28, 2016

ivovandongen force-pushed the 6273-fs-filesource branch from a8d0eee to 97ebe13 Compare September 28, 2016 15:36

ivovandongen force-pushed the 6273-fs-filesource branch from 97ebe13 to 0c836e6 Compare September 29, 2016 10:44

ivovandongen changed the title ~~file:// FileSource~~ LocalFileSource Sep 29, 2016

tmpsantos reviewed Sep 29, 2016

View reviewed changes

ivovandongen force-pushed the 6273-fs-filesource branch from a28508a to 4427633 Compare September 29, 2016 15:08

ivovandongen force-pushed the 6273-fs-filesource branch from 4427633 to d0755b6 Compare September 29, 2016 17:01

tmpsantos reviewed Sep 29, 2016

View reviewed changes

ivovandongen force-pushed the 6273-fs-filesource branch from d0755b6 to 9c3cf32 Compare September 29, 2016 17:12

tmpsantos reviewed Sep 29, 2016

View reviewed changes

ivovandongen force-pushed the 6273-fs-filesource branch from 9c3cf32 to 983d359 Compare September 30, 2016 07:46

ivovandongen added 4 commits September 30, 2016 12:09

[core] LocalFileSource implementation and tests

ede9096

[android][ios][macos][linux][qt] included default LocalFileSource

f651bc8

[darwin] NSURL remove workaround for file urls

5bec721

[android] file:// example

39dfb4d

ivovandongen force-pushed the 6273-fs-filesource branch from 983d359 to 39dfb4d Compare September 30, 2016 10:09

tmpsantos approved these changes Sep 30, 2016

View reviewed changes

ivovandongen merged commit d4888cb into master Sep 30, 2016

ivovandongen deleted the 6273-fs-filesource branch September 30, 2016 12:57

ivovandongen mentioned this pull request Oct 7, 2016

Add test app activity for loading a local style.json file #6265

Closed

cammace mentioned this pull request Oct 18, 2016

Local style source file example mapbox/mapbox-android-demo#165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LocalFileSource #6497

LocalFileSource #6497

ivovandongen commented Sep 28, 2016

mention-bot commented Sep 28, 2016

ivovandongen commented Sep 28, 2016

1ec5 commented Sep 29, 2016

1ec5 commented Sep 29, 2016

brunoabinader commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

ivovandongen commented Sep 29, 2016

tmpsantos Sep 29, 2016

tmpsantos Sep 29, 2016

tmpsantos Sep 29, 2016

tmpsantos Sep 29, 2016

tmpsantos Sep 29, 2016 •

edited

Loading

ivovandongen commented Sep 29, 2016

tmpsantos Sep 29, 2016

ivovandongen Sep 29, 2016

tmpsantos Sep 29, 2016

1ec5 commented Sep 29, 2016

ivovandongen commented Sep 30, 2016

tmpsantos left a comment

tobrun commented Sep 30, 2016

ericpalakovichcarr commented Sep 30, 2016

zugaldia commented Sep 30, 2016

SeanChristopherConway commented Jan 16, 2017

1ec5 commented Jan 16, 2017


		namespace {

		std::string toAbsoluteUrl(const std::string& fileName) {

LocalFileSource #6497

LocalFileSource #6497

Conversation

ivovandongen commented Sep 28, 2016

mention-bot commented Sep 28, 2016

ivovandongen commented Sep 28, 2016

1ec5 commented Sep 29, 2016

1ec5 commented Sep 29, 2016

brunoabinader commented Sep 29, 2016

tmpsantos commented Sep 29, 2016

ivovandongen commented Sep 29, 2016

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

ivovandongen commented Sep 29, 2016

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

ivovandongen Sep 29, 2016

Choose a reason for hiding this comment

tmpsantos Sep 29, 2016

Choose a reason for hiding this comment

1ec5 commented Sep 29, 2016

ivovandongen commented Sep 30, 2016

tmpsantos left a comment

Choose a reason for hiding this comment

tobrun commented Sep 30, 2016

ericpalakovichcarr commented Sep 30, 2016

zugaldia commented Sep 30, 2016

SeanChristopherConway commented Jan 16, 2017

1ec5 commented Jan 16, 2017

tmpsantos Sep 29, 2016 •

edited

Loading