auto-detect json from response #1346

7malikk · 2023-02-05T14:48:25Z

Fixes #1343

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

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
code is in a uniquely-named feature branch and has no merge conflicts 📁
screenshots/GIFs are attached 📎 in case of UI updates
@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 · 2023-02-05T14:56:13Z

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 · 2023-02-05T15:49:47Z

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

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!)
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 · 2023-02-05T15:50:14Z

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

7malikk · 2023-02-05T15:53:35Z

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.

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 · 2023-02-05T15:54:41Z

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 · 2023-02-05T15:59:45Z

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 <#1346 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J4OHTKKEIFBX4PD3ULWV7EM3ANCNFSM6AAAAAAUR2AG4I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

7malikk · 2023-02-05T16:04:37Z

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 · 2023-02-05T16:46:20Z

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 <#1346 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JYNOFMZM5V7ECTKDCTWV7FR7ANCNFSM6AAAAAAUR2AG4I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

7malikk · 2023-02-05T16:51:25Z

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 · 2023-02-05T20:02:20Z

@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 · 2023-02-05T23:07:33Z

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!

jywarren · 2023-02-05T23:11:42Z

examples/js/archive.js


 function showImages(getUrl) {
  const url = getUrl.replace('details', 'metadata');

  axios.get(url)
-      .then((response) => {
+    .then((response) => {
+        loadJSONfromResponse(response)


This looks great, but I'm seeing this error when entering the url https://archive.org/details/sanborn-1885

DistortableCollection.js:226 err TypeError: Cannot read properties of undefined (reading 'length')

The JSON in this is mapknitter.json and the contents are:

[{"id":1504,"src":"https://archive.org/download/sanborn-1885/sanborn-1885.jpg", "width":7573, "height":6396, "image_file_name":"sanborn-1885.jpg", "nodes":[ {"lat":39.32869582897733,"lon":-120.187548995018}, {"lat":39.32982863334289,"lon":-120.18338084220888}, {"lat":39.3270941035765,"lon":-120.182141661644}, {"lat":39.32596125491416,"lon":-120.18630981445314} ],"cm_per_pixel":10.875006010132818}]

What do you think is happening? Ah, look:

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'avg_cm_per_pixel') at reconstructMapFromJson (archive.js:254:53)

Maybe we have to check for that first, in case it doesn't exist?

Great work so far!!!

Oh alright, I'll add that check

Although I have an idea, I'd explore it and get back to you

7malikk · 2023-02-06T14:27:43Z

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

jywarren · 2023-02-06T20:58:25Z

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

7malikk · 2023-02-06T21:07:35Z

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 · 2023-02-06T22:51:12Z

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?

Leaflet.DistortableImage/src/DistortableCollection.js

Line 215 in 8cca752

avg_cm_per_pixel: response.data.avg_cm_per_pixel,

7malikk · 2023-02-07T10:51:10Z

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 · 2023-02-07T12:59:04Z

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:

Leaflet.DistortableImage/src/DistortableCollection.js

Lines 209 to 210 in 8cca752

    
           if (response.data.images.length > 1) { 
        
             response.data.images.forEach((data) => {

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 · 2023-02-07T15:25:22Z

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:

Leaflet.DistortableImage/src/DistortableCollection.js

Lines 209 to 210 in 8cca752

if (response.data.images.length > 1) {

response.data.images.forEach((data) => {

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 #1345

7malikk · 2023-02-07T15:28:57Z

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 · 2023-02-09T12:55:32Z

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 · 2023-02-09T13:30:12Z

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 <#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 <#1346 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J5ACTKSUJXV2MIJ6Z3WWTSM7ANCNFSM6AAAAAAUR2AG4I> . You are receiving this because you were mentioned.Message ID: ***@***.***>

7malikk · 2023-02-13T11:08:44Z

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

auto-detect json from response

90425b0

7malikk added 2 commits February 5, 2023 20:50

standardized name with unique ID

c5e25d4

quick fix

321f22c

jywarren reviewed Feb 5, 2023

View reviewed changes

hot fix

8629571

avg_cm_per_pixel optionally available

19e59e0

fixes

79ecb48

7malikk closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto-detect json from response #1346

auto-detect json from response #1346

7malikk commented Feb 5, 2023 •

edited

Loading

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023

jywarren commented Feb 5, 2023

7malikk commented Feb 5, 2023

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 via email

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 via email

7malikk commented Feb 5, 2023

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 •

edited

Loading

jywarren Feb 5, 2023

7malikk Feb 6, 2023

7malikk commented Feb 6, 2023

jywarren commented Feb 6, 2023

7malikk commented Feb 6, 2023

jywarren commented Feb 6, 2023

7malikk commented Feb 7, 2023

7malikk commented Feb 7, 2023

segun-codes commented Feb 7, 2023

or

7malikk commented Feb 7, 2023

7malikk commented Feb 9, 2023

jywarren commented Feb 9, 2023 via email

7malikk commented Feb 13, 2023

auto-detect json from response #1346

auto-detect json from response #1346

Conversation

7malikk commented Feb 5, 2023 • edited Loading

UI changes:

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023

jywarren commented Feb 5, 2023

7malikk commented Feb 5, 2023

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 via email

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 via email

7malikk commented Feb 5, 2023

7malikk commented Feb 5, 2023

jywarren commented Feb 5, 2023 • edited Loading

jywarren Feb 5, 2023

Choose a reason for hiding this comment

7malikk Feb 6, 2023

Choose a reason for hiding this comment

7malikk commented Feb 6, 2023

jywarren commented Feb 6, 2023

7malikk commented Feb 6, 2023

jywarren commented Feb 6, 2023

7malikk commented Feb 7, 2023

7malikk commented Feb 7, 2023

or

segun-codes commented Feb 7, 2023

or

7malikk commented Feb 7, 2023

7malikk commented Feb 9, 2023

jywarren commented Feb 9, 2023 via email

7malikk commented Feb 13, 2023

7malikk commented Feb 5, 2023 •

edited

Loading

jywarren commented Feb 5, 2023 •

edited

Loading