[ios] Undo for the bookmark and tracks deletion #8263

kirylkaveryn · 2024-05-23T16:09:23Z

This PR contains the implementation of Undo for the bookmarks and tracks deletion.
The Undo button was added to the Toast message so the user has 3 seconds to undo deletion.
When a bookmark is created from the map, a deleted list is checked first. If there's already a bookmark at the same coordinates, its info is taken to fill a new bookmark, and it is removed from a "deleted" list.

Todo:

Bookmark deletion undo
Track deletion undo
Toast message redesign to support Undo button
Localize the toast texts
When a bookmark is created from the map, a deleted list is checked first. If there's already a bookmark at the same coordinates, its info is taken to fill a new bookmark, and it is removed from a "deleted" list.
Reload the Collections list, Bookmarks list and Place Page when the undo happens

Screnrecords:

biodranik · 2024-05-23T21:55:07Z

Thanks! How does it work when bookmark is deleted on the map? This is the main case of issues.

kirylkaveryn · 2024-05-24T06:13:05Z

Thanks! How does it work when bookmark is deleted on the map? This is the main case of issues.

For now it is only the local solution to test the undo flow.

To generalise the BM deletion may be tricky, because we need to store the deleted bookmarks somewhere in code. I'll try to move the undo manager into the bookmarks manager and to add an observer for adding the bookmark.

Should we store them only for some seconds? On during the whole session?

biodranik · 2024-05-24T06:59:46Z

It would be great if the same approach/code could also be reused on Android without many changes.

One of the ideas was to save all the recently deleted bookmark info so it will be automatically restored back when it's bookmarked again. Then, for example, after someone deletes a bookmark from the map, a toast with the hint "bookmark it again to restore" can be displayed 2 or 3 times. In this case, the bookmark can stay in memory until the app is restarted.

Storing only for some seconds may be more complex than storing for the whole session.

kirylkaveryn · 2024-05-24T07:56:03Z

Then, for example, after someone deletes a bookmark from the map, a toast with the hint "bookmark it again to restore" can be displayed 2 or 3 times

Maybe do not distract users with additional toasts? It may be annoyng. I think that one with the Undo will be enogh.

it will be automatically restored back when it's bookmarked again

Such kind of restoring requires == operator to the bookmarks (as we discussed in several topics) to find the bookmark in the recently deleted list (by cooordinates?) and an additional logic in the code... Maybe we can skip the "bookmark it again to restore" in the first implementation of this feature?

biodranik · 2024-05-25T13:02:08Z

WDYT about about the following approach?

When a bookmark is deleted (on the map or the list, doesn't matter), it is placed into a memory-only list of deleted bookmarks in the C++ core.
This memory-only list will be cleared on the app restart.
Bookmarks deleted when a whole list is removed should not be cached in this way. Deleted lists should have a different undo mechanism.
When a bookmark is created from the map, a deleted list is checked first. If there's already a bookmark at the same coordinates, its info is taken to fill a new bookmark, and it is removed from a "deleted" list.

There are no any UI changes here, it should just work on both platforms.

rtsisyk · 2024-05-28T12:13:33Z

CC: @oleg-rswll, this is a new feature in progress.

kirylkaveryn · 2024-05-30T11:05:07Z

@biodranik @vng can you please check the RP?
I've implemented all related to recovery code in c++.

Some additional questions:

the toast message needs a proper localized format. In Russian for example may be unclear: "<имя метки/трека> был(а/о) удален(а/о)". How can you see the message?

kirylkaveryn · 2024-05-30T17:20:55Z

The branch was updated and I removed the

When a bookmark is created from the map, a deleted list is checked first. If there's already a bookmark at the same coordinates, its info is taken to fill a new bookmark, and it is removed from a "deleted" list.

to simplify the behaviour.

biodranik · 2024-05-30T18:23:09Z

How will it look if the toast message is removed on the map, and the "Save" text on the bottom button changes to "Restore" after a bookmark is deleted (until the selection stays active)?

I'm not sure yet about the undo in the lists. While it looks cool, nobody complained yet so far about it. This deletion requires 2 explicit actions or an explicit Delete button press, so it is highly unlikely to happen accidentally.

Is it possible to review/merge the on-the-map undo part separately?

kirylkaveryn · 2024-05-30T18:48:18Z

How will it look if the toast message is removed on the map, and the "Save" text on the bottom button changes to "Restore" after a bookmark is deleted (until the selection stays active)?

This feature requires a huge refactoring of the action bar and place page data in the core... I can try to implement it.

I'm not sure yet about the undo in the lists. While it looks cool, nobody complained yet so far about it. This deletion requires 2 explicit actions or an explicit Delete button press, so it is highly unlikely to happen accidentally.

The bookmarks can be easily deleted by trailing swipe with 1 accidental touch.

I faced sometimes with it by my own.

The overall approach with toast doesn't look disturbing for me... You can check the flow.

trim.A86D79F0-7E61-4704-9497-2B6F0353B588.MOV

Is it possible to review/merge the on-the-map undo part separately?

I'll create the separate PR with this feature.

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

- Now Toast is a facade to hide the subclass implementations from the callers - New `UndoToast` class Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

oleg-rswll · 2024-06-03T04:37:23Z

@kirylkaveryn the approach in the video with the delete undo in the toast from Bookmarks and Tracks, looks like a great approach! It preserves the existing flow, and the toast is unintrusive and helpful.

oleg-rswll · 2024-06-03T04:38:38Z

@kirylkaveryn can we do the same thing in the Place Page?

rtsisyk

This feature looks intuitive to me. I can't say the toast message is annoying. Thanks for implementing!!!

biodranik

Note that #8352 implements Undo for accidental bookmarks deletion in the Place Page in a better/simpler way.

The higher priority task is to avoid accidental deletion (or implement undo) for a whole list of bookmarks.

After it is implemented, we can come back to this issue and re-evaluate it, considering that there were no any user complaints about accidentally deleting bookmarks or tracks from lists (it requires an explicit user action, so the chance of doing it accidentally is extremely low, compared to accidental removal of a list or accidental removal of a bookmark on the Place Page).

rtsisyk · 2024-06-11T09:29:57Z

The higher priority task is to avoid accidental deletion (or implement undo) for a whole list of bookmarks.

Sorry, the presense of another task is not a reasonable argument to reject this PR.

Note that #8352 implements Undo for accidental bookmarks deletion in the Place Page in a better/simpler way.

#8352 doesn't really solve the issue with accidential deletion of bookmarks from Bookmark screen. Yes, I understand that I can seatch for the same place on the map and re-add it to my bookmarks. Meanwhile, it was possible before #8352. The issue is that you really remember where place was. This is purpose of bookmarks - to save places you can't remember in your head. This PR solves this issue.

biodranik · 2024-06-11T14:18:01Z

While the idea looks good, in reality, we didn't have any complaints about it. This PR is not rejected, it is postponed until we get a better understanding of the real user needs.

oleg-rswll · 2024-06-11T21:43:40Z

Doing undo by adding a bookmark again is essentially hiding the functionality, there is no way to discover that this is actually intended as an undo feature. Even if someone does discover it by accident, the behavior is more similar to a bug rather than a feature.

The thinking process may be "I deleted an old bookmark because I didn't need it anymore. Then wanted to add it, just in case, but for some reason it did't clear my old notes."

rtsisyk · 2024-06-20T20:47:48Z

Could you please rebase this PR? I would like to re-test it.

rtsisyk · 2024-06-21T10:20:18Z

Discussed that rebasing will take ~1 day to finish due to accumulated changes from other PRs :(

biodranik · 2024-06-21T21:30:06Z

We discussed with Kiryl that using undo toasts is not a common pattern on iOS. The toast makes information under it inaccessible, and elements untappable. And there is a higher priority issue when a whole list of tracks and bookmarks is deleted accidentally.

We can return to this PR when the priority of this issue will change.

kirylkaveryn added iOS iOS development UX User eXperience, an issue with usability UI User interface issues labels May 23, 2024

kirylkaveryn requested a review from biodranik May 23, 2024 16:09

kirylkaveryn force-pushed the ios/undo-for-the-bookmark-deletion branch from a43533d to 9251646 Compare May 30, 2024 10:54

kirylkaveryn requested a review from vng May 30, 2024 11:01

kirylkaveryn marked this pull request as ready for review May 30, 2024 11:24

kirylkaveryn force-pushed the ios/undo-for-the-bookmark-deletion branch from 9251646 to 3ded491 Compare May 30, 2024 17:16

kirylkaveryn requested a review from rtsisyk May 30, 2024 17:22

kirylkaveryn added 5 commits May 31, 2024 17:51

[bookmarks] implement recently deleted bookmarks and recovery support

d68e204

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

[ios] add onBookmarksCategoryUpdated observing to BM manager

58de46a

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

[ios] add Recovery support to the BM manager

82a6126

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

[ios] refactor the Toast to support Undo

91b3e09

- Now Toast is a facade to hide the subclass implementations from the callers - New `UndoToast` class Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

[ios] implement Recovery for the Recently Deleted bookmarks

628de3b

Signed-off-by: Kiryl Kaveryn <kirylkaveryn@gmail.com>

kirylkaveryn force-pushed the ios/undo-for-the-bookmark-deletion branch from 3ded491 to 628de3b Compare May 31, 2024 13:52

rtsisyk approved these changes Jun 4, 2024

View reviewed changes

biodranik requested changes Jun 5, 2024

View reviewed changes

rtsisyk mentioned this pull request Jun 21, 2024

[android][ios] Add confirmation for the list deletion #1026

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Undo for the bookmark and tracks deletion #8263

[ios] Undo for the bookmark and tracks deletion #8263

kirylkaveryn commented May 23, 2024 •

edited

Loading

biodranik commented May 23, 2024

kirylkaveryn commented May 24, 2024 •

edited

Loading

biodranik commented May 24, 2024

kirylkaveryn commented May 24, 2024 •

edited

Loading

biodranik commented May 25, 2024

rtsisyk commented May 28, 2024

kirylkaveryn commented May 30, 2024 •

edited

Loading

kirylkaveryn commented May 30, 2024 •

edited

Loading

biodranik commented May 30, 2024

kirylkaveryn commented May 30, 2024 •

edited

Loading

oleg-rswll commented Jun 3, 2024 •

edited

Loading

oleg-rswll commented Jun 3, 2024

rtsisyk left a comment

biodranik left a comment

rtsisyk commented Jun 11, 2024

biodranik commented Jun 11, 2024

oleg-rswll commented Jun 11, 2024

rtsisyk commented Jun 20, 2024

rtsisyk commented Jun 21, 2024

biodranik commented Jun 21, 2024

[ios] Undo for the bookmark and tracks deletion #8263

Are you sure you want to change the base?

[ios] Undo for the bookmark and tracks deletion #8263

Conversation

kirylkaveryn commented May 23, 2024 • edited Loading

Todo:

Screnrecords:

biodranik commented May 23, 2024

kirylkaveryn commented May 24, 2024 • edited Loading

biodranik commented May 24, 2024

kirylkaveryn commented May 24, 2024 • edited Loading

biodranik commented May 25, 2024

rtsisyk commented May 28, 2024

kirylkaveryn commented May 30, 2024 • edited Loading

kirylkaveryn commented May 30, 2024 • edited Loading

biodranik commented May 30, 2024

kirylkaveryn commented May 30, 2024 • edited Loading

oleg-rswll commented Jun 3, 2024 • edited Loading

oleg-rswll commented Jun 3, 2024

rtsisyk left a comment

Choose a reason for hiding this comment

biodranik left a comment

Choose a reason for hiding this comment

rtsisyk commented Jun 11, 2024

biodranik commented Jun 11, 2024

oleg-rswll commented Jun 11, 2024

rtsisyk commented Jun 20, 2024

rtsisyk commented Jun 21, 2024

biodranik commented Jun 21, 2024

kirylkaveryn commented May 23, 2024 •

edited

Loading

kirylkaveryn commented May 24, 2024 •

edited

Loading

kirylkaveryn commented May 24, 2024 •

edited

Loading

kirylkaveryn commented May 30, 2024 •

edited

Loading

kirylkaveryn commented May 30, 2024 •

edited

Loading

kirylkaveryn commented May 30, 2024 •

edited

Loading

oleg-rswll commented Jun 3, 2024 •

edited

Loading