auto-detect json from response

[7malikk]

Fixes #1343

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

• [x] PR is descriptively titled 📑 and links the original issue above 🔗
• [ ] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
• [x] code is in a uniquely-named feature branch and has no merge conflicts 📁
• [x] screenshots/GIFs are attached 📎 in case of UI updates
• [x] @mention the original creator of the issue in a comment below for help or for a review

UI changes:

When there are multiple JSON files in URL response

When there is just one JSON file in URL response

[7malikk]

Hello @jywarren, in trying to address the solution using the method in #1343, I came across a small disparity.

IA returns JSON files like so: With this, I could not verify if the JSON file contains images or not.

So I pivoted to construct the URL and pass it to the reconstructMapFromURL() function here

What are your thoughts?

[jywarren]

Ah I see, that makes sense, we don't see the contents. We could try two things:

1. Download and test each JSON file (could be a lot of requests but probably won't be, people don't store a lot of JSON files in IA!)
2. Have a consistent name for the JSON file, kind of like package.json or something. We could call it mapknitter.json?

What do you prefer? We could also have it search for mapknitter.json first but if it doesn't find it, try all other JSON files?

[jywarren]

Or we could let people choose one from a list, but that's a lot of extra UI to design! 😅

[7malikk]

1. Download and test each JSON file (could be a lot of requests but probably won't be, people don't store a lot of JSON files in IA!)

By download, do you mean we should load it on the tile layer? I'm not sure I understand.

2. Have a consistent name for the JSON file, kind of like package.json or something. We could call it mapknitter.json?

Alright, I understand this

What do you prefer? We could also have it search for mapknitter.json first but if it doesn't find it, try all other JSON files?

Okay, so do you mean if there's no Json file named mapknitter.json it should load whatever Json file is available?

[7malikk]

Or we could let people choose one from a list, but that's a lot of extra UI to design! 😅

😅😅 That's right.

So what's your final say, I'm still trying to wrap my head around the previous comment

[jywarren]

I'm not sure! Maybe the easiest way for now is to standardize the name. Then we can add full auto detection later?

On Sun, Feb 5, 2023, 10:54 AM Malik @.***> wrote:

Or we could let people choose one from a list, but that's a lot of extra UI to design! 😅

😅😅 That's right.

So what's your final say, I'm still trying to wrap my head around the previous comment

— Reply to this email directly, view it on GitHub https://github.com/publiclab/Leaflet.DistortableImage/pull/1346#issuecomment-1418076156, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J4OHTKKEIFBX4PD3ULWV7EM3ANCNFSM6AAAAAAUR2AG4I . You are receiving this because you were mentioned.Message ID: @.***>

[7malikk]

I'm not sure! Maybe the easiest way for now is to standardize the name. Then we can add full auto detection later?

Sounds good to me. What of cases of multiple files?

Also should standardization remove the need to prompt the user for a file name?

And I believe this will affect the download button too, so the naming should be standardized for now

[jywarren]

What if it just has to start with "mapknitter" but could also contain later characters like date, a more specific label. So mapknitter-__.json. And we just use the first one we find, for now.

On Sun, Feb 5, 2023, 11:04 AM Malik @.***> wrote:

I'm not sure! Maybe the easiest way for now is to standardize the name. Then we can add full auto detection later?

Sounds good to me. What of cases of multiple files?

Also should standardization remove the need to prompt the user for a file name?

And I believe this will affect the download button too, so the naming should be standardized for now

— Reply to this email directly, view it on GitHub https://github.com/publiclab/Leaflet.DistortableImage/pull/1346#issuecomment-1418088599, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6JYNOFMZM5V7ECTKDCTWV7FR7ANCNFSM6AAAAAAUR2AG4I . You are receiving this because you were mentioned.Message ID: @.***>

[7malikk]

What if it just has to start with "mapknitter" but could also contain later characters like date, a more specific label. So mapknitter-__.json. And we just use the first one we find, for now.

Okay, I think that makes sense

So the first match for the search of "mapknitter" should be loaded.

[7malikk]

@jywarren I made the changes to have a standardized name of mapknitter with a unique ID generated from the time it was downloaded resulting in a filename of: mapknitter-1293929493 like so:

I also updated the JSON filter to check for files that start with "mapknitter" and are in JSON format and prompt the user. If the user confirms to load it, it is then loaded on the map like so:

[jywarren]

This sounds great. What if the random number is a timestamp? Or a date time formatted string like 20:00-01-04-2023 or something?

Testing your code out now!

[7malikk]

@jywarren I've added the check for the avg_cm_per_pixel though it looks a tad hacky, what do you think?

[jywarren]

Hmm, can't we still reconstruct without this value? Where are we depending on it?

[7malikk]

Hmm, can't we still reconstruct without this value? Where are we depending on it?

I wondered the same thing we do make use of it here, but I am yet to answer the question if the reconstruction from JSON is dependent on that value

[jywarren]

I want to say we are providing it optionally. Instead of rejecting if we don't have the value, shall we make the change in the following area to only include that value in the output if it exists, otherwise we don't set that property on line 215?

https://github.com/publiclab/Leaflet.DistortableImage/blob/8cca752ca55b4aaa5ccedcb2689589e0f2eab1ff/src/DistortableCollection.js#L215

[7malikk]

I want to say we are providing it optionally. Instead of rejecting if we don't have the value, shall we make the change in the following area to only include that value in the output if it exists, otherwise we don't set that property on line 215?

Alright, I will give it a shot @jywarren

[7malikk]

I want to say we are providing it optionally. Instead of rejecting if we don't have the value, shall we make the change in the following area to only include that value in the output if it exists, otherwise we don't set that property on line 215?

While attempting to do this, I noticed that the format which the JSON file I saved is integral to how it would be reconstructed based on the setup here:

https://github.com/publiclab/Leaflet.DistortableImage/blob/8cca752ca55b4aaa5ccedcb2689589e0f2eab1ff/src/DistortableCollection.js#L209-L210

I propose we stick to a single format of saving JSON files, our choices are :

{"avg_cm_per_pixel":7.291666666666667,
"images":[{image1}, {image2}]
}

or

[{image1}, {image2}]

What do you think @jywarren?

[segun-codes]

I want to say we are providing it optionally. Instead of rejecting if we don't have the value, shall we make the change in the following area to only include that value in the output if it exists, otherwise we don't set that property on line 215?

While attempting to do this, I noticed that the format which the JSON file I saved is integral to how it would be reconstructed based on the setup here:

https://github.com/publiclab/Leaflet.DistortableImage/blob/8cca752ca55b4aaa5ccedcb2689589e0f2eab1ff/src/DistortableCollection.js#L209-L210

I propose we stick to a single format of saving JSON files, our choices are :

{"avg_cm_per_pixel":7.291666666666667,
"images":[{image1}, {image2}]
}

or

[{image1}, {image2}]

What do you think @jywarren?

Oh! @7malikk, good observation. Your question is similar to what we are discussing here https://github.com/publiclab/Leaflet.DistortableImage/pull/1345

[7malikk]

Oh! @7malikk, good observation. Your question is similar to what we are discussing here #1345

Really? @segun-codes, let me go give it a read through

[7malikk]

Hello, @jywarren I've made avg_cm_per_pixel an optional property in reconstructing the map from JSON URL. I took into consideration changes from #1345 which upon final works, this PR would be ready for merge after some conflict resolutions

[jywarren]

Ok great! Let me know and I'll test and merge!

On Thu, Feb 9, 2023, 7:55 AM Malik @.***> wrote:

Hello, @jywarren https://github.com/jywarren I've made avg_cm_per_pixel an optional property in reconstructing the map from JSON URL. I took into consideration changes from #1345 https://github.com/publiclab/Leaflet.DistortableImage/pull/1345 which upon final works, this PR would be ready for merge after some conflict resolutions

— Reply to this email directly, view it on GitHub https://github.com/publiclab/Leaflet.DistortableImage/pull/1346#issuecomment-1424150858, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAF6J5ACTKSUJXV2MIJ6Z3WWTSM7ANCNFSM6AAAAAAUR2AG4I . You are receiving this because you were mentioned.Message ID: @.***>

[7malikk]

@jywarren this PR is refactored at #1351, I'll be closing this one