save map coordinates in local storage

vanithaak commented 2 years ago

Fixes #998 (<=== Add issue number here)

Co-authored by: @7malikk

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 🔗
[x] 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 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

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

GIF

https://user-images.githubusercontent.com/59090053/197402978-eafa56f2-5d82-4ba1-9798-e6404b712c98.mp4

gitpod-io[bot] commented 2 years ago

vanithaak commented 2 years ago

@jywarren , @TildaDares please review our pr

vanithaak commented 2 years ago

@jywarren , please review this pr, not #1220

leilayesufu commented 2 years ago

Hii @vanithaak Sure I’d love to work with you

vanithaak commented 2 years ago

Hii @vanithaak Sure I’d love to work with you

Hey! Can we start working on it after you and me are done writing the proposal? For now, the proposal looks like a big task to me, haha

vanithaak commented 2 years ago

Hi @leilayesufu , can you please send me a Hi in gitter? It will be easy to talk there. you can find me as @vanithaak

leilayesufu commented 2 years ago

Alright on it!

vanithaak commented 2 years ago

Alright on it!

I couldn't find you, that's why 😅

7malikk commented 1 year ago

@jywarren I was going through this PR as it's one of the tasks I'll be handling as agreed with @segun-codes

I believe it's a good step in the right direction but after going through the comments, I noticed it is overlapping with #1161. Could you clarify the situation as the comments were a tad confusing between both PRs.

After which @vanithaak, @leilayesufu, and I could resolve them both and make them merge-worthy if they are still willing to.

Cheers.

jywarren commented 1 year ago

Yes, thank you. I think ultimately, the steps for this and for downloading a JSON file are the same up to the point that generateExportJson() is run. That still needs an extra parameter to include all images, not just selected ones: https://github.com/publiclab/Leaflet.DistortableImage/pull/1161#issuecomment-1296035190

However once that's complete (perhaps it needs its own PR?) then both these projects can rely on it to achieve their goals.

However, I wonder a couple things. The use case for saving the whole map is pretty good, i think -- i.e. you make a map, you want to continue another day, OR you accidentally close the tab, and you want to resume.

We could scan for recent saved JSON by skimming through saves, maybe we use the key savedMapKnitterMap and the value is the timestamp? then prompt the user by saying "A map was recently autosaved, would you like to continue?" This itself could all be a separate PR -- this PR could be focused just on saving the JSON. But just thinking ahead to know why we're doing this all.

How does that sound?

jywarren commented 1 year ago

Another question is WHEN we should be saving the JSON. Perhaps we save it every time an image is moved or changed, but, do we overwrite the last change? Or are we constantly adding "saves"?

The other use case, which I think is separate, is to try to uniquely identify each image, perhaps based on filename, file size, dimensions? or some other kind of "fingerprint"? The goal would in that case be to "recognize" an image that's been placed before, and offer to recover its location. I could imagine various UIs for that. But it seems a different enough scenario that we should just not mix it up with this. It maybe has more to do with #1171 and #1162 / https://github.com/publiclab/Leaflet.DistortableImage/pull/1236

7malikk commented 1 year ago

Yes, thank you. I think ultimately, the steps for this and for downloading a JSON file are the same up to the point that generateExportJson() is run. That still needs an extra parameter to include all images, not just selected ones: #1161 (comment)

However once that's complete (perhaps it needs its own PR?) then both these projects can rely on it to achieve their goals.

However, I wonder a couple things. The use case for saving the whole map is pretty good, i think -- i.e. you make a map, you want to continue another day, OR you accidentally close the tab, and you want to resume.

We could scan for recent saved JSON by skimming through saves, maybe we use the key savedMapKnitterMap and the value is the timestamp? then prompt the user by saying "A map was recently autosaved, would you like to continue?" This itself could all be a separate PR -- this PR could be focused just on saving the JSON. But just thinking ahead to know why we're doing this all.

How does that sound?

Alright, for better understanding let me try to explain what I understand from your comment, correct me if I am wrong.

Due to the fact that the download as JSON file and save in localStorage use the same steps up until it get's to generateExportJson(), we need to add a parameter as mention here: https://github.com/publiclab/Leaflet.DistortableImage/pull/1161#issuecomment-1296035190 of which save in localStorage and download as JSON file can both rely on. And from the comment, the parameter will be onlySelected which if false, we get all the images? and if isCollected is true we get only the selected images? I believe i need more clarification here.

Moving down to the use case of saving the most recent maps via adding a key to the JSON file named savedMapKnitterMap and value is its timestamp. this makes sense to me so far. more of a grand picture. i believe i get the idea. this far

7malikk commented 1 year ago

Another question is WHEN we should be saving the JSON. Perhaps we save it every time an image is moved or changed, but, do we overwrite the last change? Or are we constantly adding "saves"?

The other use case, which I think is separate, is to try to uniquely identify each image, perhaps based on filename, file size, dimensions? or some other kind of "fingerprint"? The goal would in that case be to "recognize" an image that's been placed before, and offer to recover its location. I could imagine various UIs for that. But it seems a different enough scenario that we should just not mix it up with this. It maybe has more to do with #1171 and #1162 / #1236

Concerning the first statement in this comment, I believe we should overwrite the last changes, as I am assuming if we are saving it, it would be in the localStorage and not locally? and localStorage as we've agreed that it is limited in space.

The unique identifier is a very good idea and I agree we should not mix it up with this. But I will note it down for when we eventually get to that PR.

What do you think? @jywarren

jywarren commented 1 year ago

Concerning the first statement in this comment, I believe we should overwrite the last changes, as I am assuming if we are saving it, it would be in the localStorage and not locally? and localStorage as we've agreed that it is limited in space.

OK, let's start by overwriting. But we can note in a comment that it could be changed in the future (if we wanted, say, to create an "undo" feature).

The unique identifier is a very good idea and I agree we should not mix it up with this. But I will note it down for when we eventually get to that PR.

Agreed! Shall we open a new issue for it, perhaps?

7malikk commented 1 year ago

OK, let's start by overwriting. But we can note in a comment that it could be changed in the future (if we wanted, say, to create an "undo" feature).

Yea, that's a good one I'll keep it in mind.

Agreed! Shall we open a new issue for it, perhaps?

Alright, @jywarren will get right on it

7malikk commented 1 year ago

1171 and #1162 / #1236

@jywarren I'd appreciate your input on this and a direction on where to begin If @vanithaak or @leilayesufu are still willing.

vanithaak commented 1 year ago

@jywarren I'd appreciate your input on this and a direction on where to begin If @vanithaak or @leilayesufu are still willing.

Hi @7malikk, thank you so much for asking. Please go ahead to work on this feature. Would love to see your work!

segun-codes commented 1 year ago

Concerning the first statement in this comment, I believe we should overwrite the last changes, as I am assuming if we are saving it, it would be in the localStorage and not locally? and localStorage as we've agreed that it is limited in space.

OK, let's start by overwriting. But we can note in a comment that it could be changed in the future (if we wanted, say, to create an "undo" feature).

The unique identifier is a very good idea and I agree we should not mix it up with this. But I will note it down for when we eventually get to that PR.

Agreed! Shall we open a new issue for it, perhaps?

@jywarren, you mirrored my thought on the consideration for supporting 'undo' operation. The 'undo' operation will only have to be limited to a lean number of save points given the memory size constraint on localStorage.

jywarren commented 1 year ago

@jywarren, you mirrored my thought on the consideration for supporting 'undo' operation. The 'undo' operation will only have to be limited to a lean number of save points given the memory size constraint on localStorage.

Agreed, we can think about it later, but I've for example used a rule like "overwrite if last save is within 5 minutes ago" and there can also be a hard rule like "limit total # of saves to 50 and delete older ones when adding a new one". Thanks!

segun-codes commented 1 year ago

@jywarren, you mirrored my thought on the consideration for supporting 'undo' operation. The 'undo' operation will only have to be limited to a lean number of save points given the memory size constraint on localStorage.

Agreed, we can think about it later, but I've for example used a rule like "overwrite if last save is within 5 minutes ago" and there can also be a hard rule like "limit total # of saves to 50 and delete older ones when adding a new one". Thanks!

Okay let's see.

7malikk commented 1 year ago

@jywarren per our conversation on saving a map to localStorage does the work @vanithaak accomplished set a good base? If yes where do i insert the alert that reads “There are N saved maps… would you like to recover…?”

jywarren commented 1 year ago

Hi, I think we need to design some kind of new interface for this. Could we show a list of saved maps and, maybe, how many images each save has, when it was saved? And have a button on each to recover? Would you like to try to create a sketch of how this UI might look? Thank you!

7malikk commented 1 year ago

Hi, I think we need to design some kind of new interface for this. Could we show a list of saved maps and, maybe, how many images each save has, when it was saved? And have a button on each to recover? Would you like to try to create a sketch of how this UI might look? Thank you!

Sure, I would like to get that done @jywarren

7malikk commented 1 year ago

Hello @jywarren I was thinking we could use a modal designed like so: Saved Maps

What do you think?

jywarren commented 1 year ago

That looks great! It could be done with bootstrap components pretty easily right?

On Tue, Feb 7, 2023, 10:12 AM Malik @.***> wrote:

Hello @jywarren https://github.com/jywarren I was thinking we could use a modal designed like so: [image: Saved Maps] https://user-images.githubusercontent.com/75104021/217283516-6af515ab-113d-4400-8f34-a3c459ee8f84.png

What do you think?

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

7malikk commented 1 year ago

That looks great! It could be done with bootstrap components pretty easily right?

Yea, i believe so

jywarren commented 1 year ago

To consider for where to place the button/link to open saved maps list modal!

jywarren commented 1 year ago

Here's the latest sidebar top UI. Are we over-complicating the "save" button or feature in introducing this new way of saving, and how could our design address a risk of "overcomplicating" to a new user?

7malikk commented 1 year ago

Here's the latest sidebar top UI. Are we over-complicating the "save" button or feature in introducing this new way of saving, and how could our design address a risk of "overcomplicating" to a new user?

I believe like you said, what we might see as giving the users options may look like complexities to the user.

But in the end I believe it'll be beneficial to the the user. They'd just have to get used to it?

What do you think? @jywarren

7malikk commented 1 year ago

To consider for where to place the button/link to open saved maps list modal!

This seems like the best spot, if we're deciding the modal does not come first

But we do have to decide on where the save to localStorage button will be if there will be one. @jywarren

7malikk commented 1 year ago

I believe like you said, what we might see as giving the users options may look like complexities to the user.

But in the end I believe it'll be beneficial to the the user. They'd just have to get used to it?

What do you think? @jywarren

Further more, if we do decide to add the save to LocalStorage, I was taking a look at the bootstrap dropdown I believe it'll suffice. Where one action could be to download locally and the other to save to local storage. @jywarren

jywarren commented 1 year ago

Where one action could be to download locally and the other to save to local storage. @jywarren

That does sound like a good idea! Would the dropdown present both options each time, or would the main button have a default action?

For example, this button has other options, but the main one is "Squash and merge". What might be the default for our Save button?

I think it might be worth making a more visible button outline (using a different bootstrap style or color) as well to make it more clear, what do you think?

jywarren commented 1 year ago

As to complexity, having a strong default can really help with that. Then, new users will just use the default. Others can use the more advanced features hidden under the dropdown. How does that sound?

7malikk commented 1 year ago

@jywarren, my initial thought was that the button would trigger the dropdown which contains the two possible actions being: download or save to localStorage. With a descriptive tooltip, even new users won't find it complex in my opinion.

Using the squash and merge type, I believe only curious users would check what the dropdown does.

7malikk commented 1 year ago

Hello @jywarren What do you think about this?

jywarren commented 1 year ago

@jywarren, my initial thought was that the button would trigger the dropdown which contains the two possible actions being: download or save to localStorage. With a descriptive tooltip, even new users won't find it complex in my opinion.

This sounds great. I am not sure that's the way Bootstrap dropdowns normally work but they can probably be configured that way!

jywarren commented 1 year ago

Regarding the diagram, i like it! Very easy to read. The part about max # of maps is also a good thing to think ahead to. I think it would be a very long time before we hit the limit though... isn't localStorage like 4 mb or something?

7malikk commented 1 year ago

This sounds great. I am not sure that's the way Bootstrap dropdowns normally work but they can probably be configured that way!

I'd get on it and see

7malikk commented 1 year ago

Regarding the diagram, i like it! Very easy to read. The part about max # of maps is also a good thing to think ahead to. I think it would be a very long time before we hit the limit though... isn't localStorage like 4 mb or something?

That's true, 4-5 mb

7malikk commented 1 year ago

Hello, @jywarren Here is the current look of the save button, What are your thoughts? I am awaiting a merge on #1345 as a lot of changes there is core.

saveMaps

jywarren commented 1 year ago

looks perfect!!

jywarren commented 1 year ago

I know we're discussing in #1357, but also just let me know when this is ready to merge!

publiclab / Leaflet.DistortableImage

save map coordinates in local storage #1225

1171 and #1162 / #1236