[ios] Implement the Recently Deleted screen to restore deleted categories #7978

kirylkaveryn · 2024-04-24T16:57:06Z

kirylkaveryn commented

2024-04-24 16:57:06 +00:00

Based on: organicmaps/organicmaps#8719

Closes (for iOS) organicmaps/organicmaps#1026

This PR contains the default implementation of the Recently Deleted Screen in iOS to restore deleted categories.

Key concepts:

~~- when the user deletes a category it will be removed from the categories list and the related file will be marked with the .deleted extension~~
~~- when the user deletes a file from the Recently Deleted list it should be totally removed from the file system~~
~~- when the user restores a category from the Recently Deleted list the .deleted ext will be removed and the file will be loaded into the bookmarks categories list~~
~~- Recently deleted button is visible only when there is nothing to restore (not implemented)~~
- when the user deletes the category file.kml and creates (or import) a new category with the same name the newly created file will have the file1.kml name to avoid conflicts with the "old" file.kml + file.kml.deleted. This allows to recover the file.kml.deleted without renaming

Use the local .Trash directory to deleted files
Deleted files shouldn't be synced to the iCloud (only for ios) -> deleted content may be different for the different devices
iCloud sync manager cannot directly remove the file. Instead, it only uses the trash mechanism to delete the file. It will help to avoid destructive behavior and unexpected bugs - it will be possible to recover the file if smth happens.

Todo:

iOS:
~~- [x] iCloud sync support~~

Screen with UI elements
~~- [x] Recover the current file by a swipe~~
Recover All files at once
~~- [x] Delete file with swipe~~
Delete All files at once
Searching files
Multiple selection to restore/delete selected files
Hide the recently deleted button when there is nothing to delete
Skip the removing for the empty categories
~~- [x] Fix removing animations for cells (optional)~~
strings and localizations
Icon (the default trash can icon is used)

Core:

Method to check is the recently deleted list is empty
Method to delete category (move to the Trash)
Method to move the recover category from the Trash
Method to get the list of removed files. ~~It would be better to get an array of elements with: category_path, category_name, and deletion_date (done)~~
Handle the name duplication conflicts on both delete and recover operations

Files App (as a reference):

Results:

Based on: https://git.omaps.dev/organicmaps/organicmaps/pulls/8719 Closes (for iOS) https://git.omaps.dev/organicmaps/organicmaps/issues/1026 This PR contains the default implementation of the Recently Deleted Screen in iOS to restore deleted categories. ### Key concepts: ~~- when the user deletes a category it will be removed from the categories list and the related file will be marked with the `.deleted` extension~~ ~~- when the user deletes a file from the `Recently Deleted` list it should be totally removed from the file system~~ ~~- when the user restores a category from the `Recently Deleted` list the `.deleted` ext will be removed and the file will be loaded into the bookmarks categories list~~ ~~- `Recently deleted` button is visible only when there is nothing to restore (not implemented)~~ ~~- when the user deletes the category `file.kml` and creates (or import) a new category with the same name the newly created file will have the `file1.kml` name to avoid conflicts with the "old" `file.kml` + `file.kml.deleted`. This allows to recover the `file.kml.deleted` without renaming~~ - Use the local `.Trash` directory to deleted files - Deleted files shouldn't be synced to the iCloud (only for ios) -> deleted content may be different for the different devices - `iCloud sync manager` cannot directly remove the file. Instead, it only uses the _trash mechanism_ to delete the file. It will help to avoid destructive behavior and unexpected bugs - it will be possible to recover the file if smth happens. ## Todo: iOS: ~~- [x] iCloud sync support~~ - [x] Screen with UI elements ~~- [x] Recover the current file by a swipe~~ - [x] Recover All files at once ~~- [x] Delete file with swipe~~ - [x] Delete All files at once - [x] Searching files - [x] Multiple selection to restore/delete selected files - [x] Hide the `recently deleted` button when there is nothing to delete - [x] Skip the removing for the empty categories ~~- [x] Fix removing animations for cells (optional)~~ - [x] strings and localizations - [x] Icon (the default trash can icon is used) Core: - [x] Method to check is the `recently deleted` list is empty - [x] Method to delete category (move to the Trash) - [x] Method to move the recover category from the Trash - [x] Method to get the list of removed files. ~~It would be better to get an array of elements with: `category_path`, `category_name`, and `deletion_date` (done)~~ - [x] Handle the name duplication conflicts on both delete and recover operations ___ Files App (as a reference): <img width="300" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/076d264d-b23f-495c-8b1f-7202874474d5"> <img width="300" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/735ed6aa-8bd7-4829-8506-3ab2aab042bb"> ___ ### Results: ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 00 39](https://github.com/organicmaps/organicmaps/assets/79797627/fbfc0fd5-616b-4144-8963-9fb6d01615c9) ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 01 00](https://github.com/organicmaps/organicmaps/assets/79797627/106ad41a-9bda-4248-b049-4a90eb338402) ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 01 13](https://github.com/organicmaps/organicmaps/assets/79797627/c50f0bb9-b8f1-4fda-88d3-4ee5781d26c3) ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 01 36](https://github.com/organicmaps/organicmaps/assets/79797627/7371d70c-00d3-404f-9bd3-9ce327fd55b8) ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 01 56](https://github.com/organicmaps/organicmaps/assets/79797627/61e2fbd3-73bf-4364-99f0-3a1623fd06f6) ![Simulator Screen Recording - iPhone 15 Pro - 2024-05-07 at 22 04 23](https://github.com/organicmaps/organicmaps/assets/79797627/848e082f-05a2-43af-b8c1-a09ccca2fd09)

biodranik (Migrated from github.com) reviewed 2024-07-16 20:16:19 +00:00

biodranik (Migrated from github.com) left a comment

The PR can be split into two, and the second one can depend on the first one. Then we can review and fix them separately, focusing on the first one.

This PR can be the second one )

The PR can be split into two, and the second one can depend on the first one. Then we can review and fix them separately, focusing on the first one. This PR can be the second one )

coding/file_writer.cpp

biodranik (Migrated from github.com) commented

2024-07-16 19:35:52 +00:00

This is a static method that can be called directly, without adding it into the FileWriter (class members are not used/needed). A separate commit/PR can also remove the unnecessary wrapper FileWriter::DeleteFileX

This is a static method that can be called directly, without adding it into the FileWriter (class members are not used/needed). A separate commit/PR can also remove the unnecessary wrapper `FileWriter::DeleteFileX`

iphone/CoreApi/CoreApi.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2024-07-16 19:38:12 +00:00

Already defined in common.xcconfig

iphone/CoreApi/CoreApi.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2024-07-16 19:37:57 +00:00

Already defined in common.xcconfig

biodranik (Migrated from github.com) commented

2024-07-16 19:38:05 +00:00

Already defined in common.xcconfig

biodranik (Migrated from github.com) commented

2024-07-16 19:38:28 +00:00

Already defined in common-debug.xcconfig

iphone/CoreApi/CoreApi.xcodeproj/project.pbxproj

biodranik (Migrated from github.com) commented

2024-07-16 19:37:53 +00:00

Already defined in common.xcconfig

iphone/Maps/Core/iCloud/SynchronizationFileWriter.swift

biodranik (Migrated from github.com) commented

2024-07-16 20:11:50 +00:00

Why is this call needed?

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

biodranik (Migrated from github.com) commented

2024-07-16 20:09:06 +00:00

Is a .Trash or any other folder for deleted files really needed? Why deleted files can't be in the same bookmarks directory, together with non-deleted files?

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2024-07-16 19:57:38 +00:00

#ifndef _MSC_VER
#include <sys/time.h>
#endif

```suggestion #ifndef _MSC_VER #include <sys/time.h> #endif ```

map/bookmark_manager.cpp

					
				@ -404,6 +404,53 @@ void BookmarkManager::ResetRecentlyDeletedBookmark()

				  m_recentlyDeletedBookmark.reset();

				}

biodranik (Migrated from github.com) commented

2024-07-16 20:12:51 +00:00

A detailed comment is needed on why is this call required. Is there any way to avoid using it?

map/bookmark_manager.cpp

					
				@ -2498,2 +2543,2 @@

				  if (deleteFile)

				    FileWriter::DeleteFileX(it->second->GetFileName());

				  auto const & filePath = it->second->GetFileName();

				  if (permanently)

biodranik (Migrated from github.com) commented

2024-07-16 19:57:17 +00:00

gettimeofday and utime are not available on Windows.

#ifdef _MSC_VER
  _utime(filePath.c_str(), nullptr);
#else
  utime(filePath.c_str(), nullptr);
#endif

gettimeofday and utime are not available on Windows. ```suggestion #ifdef _MSC_VER _utime(filePath.c_str(), nullptr); #else utime(filePath.c_str(), nullptr); #endif ```

map/map_tests/bookmarks_test.cpp

biodranik (Migrated from github.com) commented

2024-07-16 19:46:21 +00:00

  std::string const filePath = base::JoinPath(dir, "file" + std::string{kKmlExtension});

```suggestion std::string const filePath = base::JoinPath(dir, "file" + std::string{kKmlExtension}); ```

biodranik (Migrated from github.com) commented

2024-07-16 19:46:36 +00:00

  kmlDataCollection.emplace_back(filePath, LoadKmlData(MemReader(kmlString, std::strlen(kmlString)), KmlFileType::Text));

```suggestion kmlDataCollection.emplace_back(filePath, LoadKmlData(MemReader(kmlString, std::strlen(kmlString)), KmlFileType::Text)); ```

biodranik (Migrated from github.com) commented

2024-07-16 19:49:01 +00:00

  auto const & deletedCategory = deletedCategories->front();

```suggestion auto const & deletedCategory = deletedCategories->front(); ```

biodranik (Migrated from github.com) commented

2024-07-16 19:49:10 +00:00

  TEST_EQUAL(base::GetFileExtension(deletedCategory.first), std::string{kDeletedExtension}, ());

```suggestion TEST_EQUAL(base::GetFileExtension(deletedCategory.first), std::string{kDeletedExtension}, ()); ```

biodranik (Migrated from github.com) commented

2024-07-16 19:49:25 +00:00

  TEST_EQUAL(deletedCategory.first, deletedFilePath, ());

```suggestion TEST_EQUAL(deletedCategory.first, deletedFilePath, ()); ```

biodranik (Migrated from github.com) commented

2024-07-16 19:50:16 +00:00

Why is it a teardown?

map/map_tests/bookmarks_test.cpp

biodranik (Migrated from github.com) commented

2024-07-16 19:48:10 +00:00

Where the restriction to delete the last list is defined? In the iOS/Android code?

kirylkaveryn reviewed 2024-07-17 08:04:36 +00:00

map/map_tests/bookmarks_test.cpp

kirylkaveryn commented

2024-07-17 08:04:36 +00:00

Yes in ios... and spread across the view controller... at least 2 times.
There is no restrictions in the core to delete the last group.
I will add a separate commit to make with a small refactoring by moving this check into the viewModel

Yes in ios... and spread across the view controller... at least 2 times. There is no restrictions in the core to delete the last group. I will add a separate commit to make with a small refactoring by moving this check into the viewModel

kirylkaveryn reviewed 2024-07-17 08:32:07 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

kirylkaveryn commented

2024-07-17 08:32:07 +00:00

The .Trash dir is needed to handle situations when the file was directly deleted from the Files app/ mac.
In this case, we faced up with the absence of the file in the icloud local container and should decide:

remove the local file
recreate the file in the cloud from the device
In this case, we inspect the cloud's trash to check that file was deleted (because files deleted manually from the Files app are moved to trash).

If there is no file in the cloud's trash the file will be recreated.

The `.Trash` dir is needed to handle situations when the file was directly deleted from the Files app/ mac. In this case, we faced up with the absence of the file in the icloud local container and should decide: 1. remove the local file 2. recreate the file in the cloud from the device In this case, we inspect the cloud's trash to check that file was deleted (because files deleted manually from the Files app are moved to trash). If there is no file in the cloud's trash the file will be recreated.

kirylkaveryn reviewed 2024-07-17 08:41:05 +00:00

iphone/Maps/Core/iCloud/SynchronizationFileWriter.swift

kirylkaveryn commented

2024-07-17 08:41:05 +00:00

When we create a file in the cloud its lastModified date will be equal to the creationDate and will not be equal to the source local file's lastModified.
In this case, we will have situation:

local file with the lastModified date 0001
copy the local file to the cloud container
cloud file (the same as local) last modified date is 0003 (now)
difference in the times will trigger the local file updation

It is important to keep the lastModified time the same as the default c++ move do (FileManager behavior is different).

When we create a file in the cloud its `lastModified` date will be equal to the `creationDate` and will **not** be equal to the source local file's `lastModified`. In this case, we will have situation: 1. local file with the lastModified date 0001 2. copy the local file to the cloud container 3. cloud file (the same as local) last modified date is 0003 (now) 4. difference in the times will trigger the local file updation It is important to keep the `lastModified` time the same as the default c++ `move` do (FileManager behavior is different).

kirylkaveryn reviewed 2024-07-17 08:51:50 +00:00

map/bookmark_manager.cpp

					
				@ -404,6 +404,53 @@ void BookmarkManager::ResetRecentlyDeletedBookmark()

				  m_recentlyDeletedBookmark.reset();

				}

kirylkaveryn commented

2024-07-17 08:51:50 +00:00

We should track the delete / recover action.

Situation:
device 1:

user deletes the category (file is marked by the .deleted ext)
could is synced
device 2:
user disables the cloud
user recovers the category (remove the .deleted ext)
user enables the could

Expectation: the file should be recovered.

But we have:
on cloud:
file.kml.deleted (lastModified 0001)
on device 1:
file.kml (lastModified 0001)

how to decide what is the latest file version?

Updating lastModified allows to keep track the file's lifecycle.
The implementation without using lastModified does not allow to track it.

The way to avoid this: create the versionig system or some kind of transactions journal.

We should track the `delete` / `recover` action. Situation: device 1: - user deletes the category (file is marked by the .deleted ext) - could is synced device 2: - user disables the cloud - user recovers the category (remove the .deleted ext) - user enables the could Expectation: the file should be recovered. But we have: on cloud: file.kml.deleted (`lastModified` 0001) on device 1: file.kml (`lastModified` 0001) how to decide what is the latest file version? Updating `lastModified` allows to keep track the file's lifecycle. The implementation without using `lastModified` does not allow to track it. The way to avoid this: create the versionig system or some kind of transactions journal.

biodranik (Migrated from github.com) reviewed 2024-07-18 20:20:31 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

biodranik (Migrated from github.com) commented

2024-07-18 20:20:31 +00:00

What are the alternative approaches and their pros/cons? E.g.

Storing "file deleted" flag in the file itself.
Storing "file deleted" record in a journal file (local or local + iCloud)

What are the alternative approaches and their pros/cons? E.g. 1. Storing "file deleted" flag in the file itself. 2. Storing "file deleted" record in a journal file (local or local + iCloud)

biodranik (Migrated from github.com) reviewed 2024-07-18 20:23:20 +00:00

iphone/Maps/Core/iCloud/SynchronizationFileWriter.swift

biodranik (Migrated from github.com) commented

2024-07-18 20:23:20 +00:00

Won't the file updation be triggered anyway before the modification time is reset to the original file's time?

kirylkaveryn reviewed 2024-07-21 07:52:09 +00:00

iphone/Maps/Core/iCloud/SynchronizationFileWriter.swift

kirylkaveryn commented

2024-07-21 07:52:09 +00:00

I've tested this behaviour - no, this update will be triggered only once because the writing operation is synchronous in the fileCoordinator's scope.

kirylkaveryn reviewed 2024-07-21 08:39:40 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

kirylkaveryn commented

2024-07-21 08:39:40 +00:00

What are the alternative approaches and their pros/cons? E.g.

Storing "file deleted" flag in the file itself.

Storing "file deleted" record in a journal file (local or local + iCloud)

the approach of storing the flags has a huge disadvantage - all the files should be downloaded first and parsed by the core before we can only check the flags/attributes. It will impact performance and mobile data consumption a lot.
This approach will allow the tracking of the file's history of changes and not rely on the file's content itself.
pros:

one source of truth of file changes and we shouldn't rely on the file's lastModifiedDate attr directly
journal is lightweight and fast to sync (like the metadata)
easier to get the file current state (deleted or not)
cons:
journal should be synced and merged first to track the file's current state
if the sync was disabled how to track the changes? This solution will not help to get the
the journal structure should be well designed and contain enough information to resolve all potential version conflicts but how much info should it contain?
should the journal know about the change's content? (Probably not - because it shouldn't replace the kml itself)

> What are the alternative approaches and their pros/cons? E.g. > > 1. Storing "file deleted" flag in the file itself. > 2. Storing "file deleted" record in a journal file (local or local + iCloud) 1. the approach of storing the flags has a huge disadvantage - all the files should be downloaded first and parsed by the core before we can **only check** the flags/attributes. It will impact performance and mobile data consumption a lot. 2. This approach will allow the tracking of the file's history of changes and not rely on the file's content itself. **pros:** - one source of truth of file changes and we shouldn't rely on the file's `lastModifiedDate` attr directly - journal is lightweight and fast to sync (like the metadata) - easier to get the file current state (deleted or not) **cons:** - journal should be synced and merged first to track the file's current state - if the sync was disabled how to track the changes? This solution will not help to get the - the journal structure should be well designed and contain enough information to resolve all potential version conflicts but how much info should it contain? - should the journal know about the change's content? (Probably not - because it shouldn't replace the kml itself) -

biodranik (Migrated from github.com) reviewed 2024-07-21 14:44:35 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

biodranik (Migrated from github.com) commented

2024-07-21 14:44:35 +00:00

Can you please elaborate on details where the overhead will occur? In this approach the latest modified file should always be downloaded and local file should be updated by a remote version, right?

1. Can you please elaborate on details where the overhead will occur? In this approach the latest modified file should always be downloaded and local file should be updated by a remote version, right?

kirylkaveryn reviewed 2024-07-21 16:01:51 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

kirylkaveryn commented

2024-07-21 16:01:51 +00:00

Can you please elaborate on the details of where the overhead will occur?

When the icloud's metadata query starts to work and sends the NSMetadataItems list to the app we should decide for every item - download and update the current file or not.

Usually, we download all the files (because all of them should be synced) and it's not a blocker.

But in case of updating the local file with the cloud's version (or vice versa) we should:

download all the cloud's files
pass them to the c++ parser to get the lastModified and deleted/not deleted attributes
compare the attributes
decide of what how to process files (update/delete/create)
process files
get a new portion of updates from the cloud with updated metadata and repeat the process from the step 2 until we see that files have equal attributes and up to date.

It may be ok process a lightweight file, but if it will be >5-10mb?
On every bookmark/color update?

Notification comes from the icloud's metadata query asynchronously and sometimes very frequently.

We cannot parse files "only once" and forget when we use >1 devices. As we see the users actively enable/disable sync and use multiple devices and this additional parsing of the cloud's contents will slow down the app performance.

We should rely on the lightweight information (file's metadata that is already composed by the cloud or custom journal to make a decision of how to process the file.

> 1. Can you please elaborate on the details of where the overhead will occur? When the icloud's metadata query starts to work and sends the NSMetadataItems list to the app we should decide for every item - download and update the current file or not. Usually, we download all the files (because all of them should be synced) and it's not a blocker. But in case of **updating** the local file with the cloud's version (or vice versa) we should: 1. download all the cloud's files 2. pass them to the c++ parser to get the lastModified and deleted/not deleted attributes 3. compare the attributes 4. decide of what how to process files (update/delete/create) 5. process files 6. get a new portion of updates from the cloud with updated metadata and repeat the process from the step 2 until we see that files have equal attributes and up to date. It may be ok process a lightweight file, but if it will be >5-10mb? On every bookmark/color update? Notification comes from the icloud's metadata query asynchronously and sometimes very frequently. We cannot parse files "only once" and forget when we use >1 devices. As we see the users actively enable/disable sync and use multiple devices and this additional parsing of the cloud's contents will slow down the app performance. We should rely on the lightweight information (file's metadata that is already composed by the cloud or custom journal to make a decision of how to process the file.

kirylkaveryn reviewed 2024-07-21 16:12:11 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

kirylkaveryn commented

2024-07-21 16:12:11 +00:00

In this approach the latest modified file should always be downloaded and local file should be updated by a remote version, right?

It's not straightforward that the remote version is newer.

There are a lot of cases when at the same time:

some cloud files should be updated with the local versions
some local files should be updated with the cloud versions
some updated/deleted/recovered

When the user starts the app or enables sync in the settings we do not have the "previous" state of the local/cloud contents but only the current and compare files without knowing "what the action triggers the change".
The cloud content may be updated from the other devices.

> In this approach the latest modified file should always be downloaded and local file should be updated by a remote version, right? It's not straightforward that the remote version is newer. There are a lot of cases when at the same time: - some cloud files should be updated with the local versions - some local files should be updated with the cloud versions - some updated/deleted/recovered When the user starts the app or enables sync in the settings we do not have the "previous" state of the local/cloud contents but only the _current_ and compare files without knowing "what the action triggers the change". The cloud content may be updated from the other devices.

biodranik (Migrated from github.com) reviewed 2024-07-21 19:47:57 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

biodranik (Migrated from github.com) commented

2024-07-21 19:47:57 +00:00

Maybe I'm not fully understanding the process. Let's discuss a simple case:

Local file was modified (written to). Its modification date was automatically updated by the system.
This file was automatically uploaded to iCloud, right?
The remote system detected that this file is newer on iCloud (is it feasible?)
The remote system downloaded it locally.

Maybe I'm not fully understanding the process. Let's discuss a simple case: 1. Local file was modified (written to). Its modification date was automatically updated by the system. 2. This file was automatically uploaded to iCloud, right? 3. The remote system detected that this file is newer on iCloud (is it feasible?) 4. The remote system downloaded it locally.

kirylkaveryn reviewed 2024-07-22 18:37:51 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

kirylkaveryn commented

2024-07-22 18:37:51 +00:00

Maybe I'm not fully understanding the process. Let's discuss a simple case:

4... on the 2nd device
5. The local file will be updated from the cloud's version (if there are no version conflicts)
All is correct.

The updation process is quite simple if the current files exist on both devices (except the situation when the sync was disabled and the file was changed on both devices).

> Maybe I'm not fully understanding the process. Let's discuss a simple case: 4... on the 2nd device 5. The local file will be updated from the cloud's version (if there are no version conflicts) All is correct. The updation process is quite simple if the current files exist on both devices (except the situation when the sync was disabled and the file was changed on both devices).

biodranik (Migrated from github.com) reviewed 2024-07-23 20:40:45 +00:00

iphone/Maps/Core/iCloud/iCloudDocumentsDirectoryMonitor.swift

biodranik (Migrated from github.com) commented

2024-07-23 20:40:45 +00:00

If this simple case works, then why storing a "deleted" flag inside a file won't work? Isn't it the same write operation as "add a bookmark" or "delete a bookmark"?

rtsisyk approved these changes 2024-08-06 08:37:35 +00:00

rtsisyk left a comment

The design from Aug 3 organicmaps/organicmaps#7978 (comment) LGTM.

The design from Aug 3 https://git.omaps.dev/organicmaps/organicmaps/pulls/7978#issuecomment-2266722453 LGTM.

biodranik (Migrated from github.com) reviewed 2024-08-13 13:58:33 +00:00

biodranik (Migrated from github.com) left a comment

It may be useful but not necessary. The search bar can be hidden during the presentation and appear on the dragging. It will save the place:

That's a cool idea!

It seems like regression and not caused by the current code. I'll fix it.

Please create a separate issue or PR for this bug.

This screen is not the same as the main list screen. It may be uncomfortable to scroll down to tap the button when there are a lot of deleted elements. And it is more UX-safe to split such important buttons stack vertically rather than horizontally (because the user can easily miss the button especially in case of delete all/ recover all). Also, we already use the same approach with the bottom bar on the several screens.

Agree, thanks for explaining your arguments!

The additional prompt will be annoying and the overall deletion process will be more complex (this is why we don't use this approach in the mail lists screen) because to delete 5 elements the user should press 10 taps (and only 6 in case of the multiple selection).

Also agree. It would be great to integrate multi selection in the main list in a natural way too, I think there was an issue about it, right?

> It may be useful but not necessary. The search bar can be hidden during the presentation and appear on the dragging. It will save the place: That's a cool idea! > It seems like regression and not caused by the current code. I'll fix it. Please create a separate issue or PR for this bug. > This screen is not the same as the main list screen. It may be uncomfortable to scroll down to tap the button when there are a lot of deleted elements. And it is more UX-safe to split such important buttons stack vertically rather than horizontally (because the user can easily miss the button especially in case of delete all/ recover all). Also, we already use the same approach with the bottom bar on the several screens. Agree, thanks for explaining your arguments! > The additional prompt will be annoying and the overall deletion process will be more complex (this is why we don't use this approach in the mail lists screen) because to delete 5 elements the user should press 10 taps (and only 6 in case of the multiple selection). Also agree. It would be great to integrate multi selection in the main list in a natural way too, I think there was an issue about it, right?

data/strings/strings.txt

biodranik (Migrated from github.com) commented

2024-08-13 13:17:36 +00:00

@organicmaps/translations Please review and check if "Lists" is translated in the same way as here for consistency:

  [bookmark_lists]
    tags = android,ios
    en = Lists
    af = Lyste
    ar = قوائم
    az = Siyahılar
    be = Спісы
    bg = Списъци
    ca = Llistes
    cs = Seznamy
    da = Lister
    de = Listen
    el = Λίστες
    es = Listas
    es-MX = Listas
    et = Loetelud
    eu = Zerrendak
    fa = لیست‌ها
    fi = Luettelot
    fr = Listes
    he = רשימות
    hi = सूचियों
    hu = Listák
    id = Daftar
    it = Elenchi
    ja = リスト
    ko = 기울기
    lt = Sąrašai
    mr = याद्या
    nb = Lister
    nl = Lijsten
    pl = Listy
    pt = Listas
    pt-BR = Listas
    ro = Liste
    ru = Списки
    sk = Zoznamy
    sv = Listor
    sw = Orodha
    th = รายการ
    tr = Listeler
    uk = Списки
    vi = Danh sách
    zh-Hans = 列表
    zh-Hant = 清單

@organicmaps/translations Please review and check if "Lists" is translated in the same way as here for consistency: ``` [bookmark_lists] tags = android,ios en = Lists af = Lyste ar = قوائم az = Siyahılar be = Спісы bg = Списъци ca = Llistes cs = Seznamy da = Lister de = Listen el = Λίστες es = Listas es-MX = Listas et = Loetelud eu = Zerrendak fa = لیست‌ها fi = Luettelot fr = Listes he = רשימות hi = सूचियों hu = Listák id = Daftar it = Elenchi ja = リスト ko = 기울기 lt = Sąrašai mr = याद्या nb = Lister nl = Lijsten pl = Listy pt = Listas pt-BR = Listas ro = Liste ru = Списки sk = Zoznamy sv = Listor sw = Orodha th = รายการ tr = Listeler uk = Списки vi = Danh sách zh-Hans = 列表 zh-Hant = 清單 ```

biodranik (Migrated from github.com) commented

2024-08-13 13:19:06 +00:00

    be = Выдаліць усё

```suggestion be = Выдаліць усё ```

biodranik (Migrated from github.com) commented

2024-08-13 13:19:21 +00:00

    ru = Удалить всё

```suggestion ru = Удалить всё ```

biodranik (Migrated from github.com) commented

2024-08-13 13:19:59 +00:00

Apple uses Recover word in Files app. We use Restore on accidentally deleted bookmarks on the map. Should the existing Restore translation for bookmarks on the map be replaced with Recover? It can be done later in a separate PR.

biodranik (Migrated from github.com) commented

2024-08-13 13:24:46 +00:00

    ru = Восстановить всё

```suggestion ru = Восстановить всё ```

iphone/CoreApi/CoreApi/Bookmarks/MWMBookmarksManager.mm

biodranik (Migrated from github.com) commented

2024-08-13 13:27:16 +00:00

  for (auto const & [filePath, categoryPtr] : * categoriesCollection) {

```suggestion for (auto const & [filePath, categoryPtr] : * categoriesCollection) { ```

iphone/Maps/Tests/Bookmarks/Categories/RecentlyDeletedTests/RecentlyDeletedCategoriesViewModelTests.swift

biodranik (Migrated from github.com) commented

2024-08-13 13:30:18 +00:00

    XCTAssertEqual(viewModel.filteredDataSource.flatMap { $0.content }.count, bookmarksManagerMock.getRecentlyDeletedCategoriesCount())

```suggestion XCTAssertEqual(viewModel.filteredDataSource.flatMap { $0.content }.count, bookmarksManagerMock.getRecentlyDeletedCategoriesCount()) ```

biodranik (Migrated from github.com) commented

2024-08-13 13:30:42 +00:00

ditto?

biodranik (Migrated from github.com) commented

2024-08-13 13:30:52 +00:00

ditto?

biodranik (Migrated from github.com) commented

2024-08-13 13:31:00 +00:00

ditto?

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2024-08-13 13:37:12 +00:00

bool BookmarkManager::IsRecentlyDeletedCategory(std::string const & filePath) const

```suggestion bool BookmarkManager::IsRecentlyDeletedCategory(std::string const & filePath) const ```

biodranik (Migrated from github.com) commented

2024-08-13 13:39:14 +00:00

Will this work?

  return filePath.find(GetTrashDirectory()) != std::string::npos;

Will this work? ```suggestion return filePath.find(GetTrashDirectory()) != std::string::npos; ```

biodranik (Migrated from github.com) commented

2024-08-13 13:42:02 +00:00

Why it should be run on UI thread? ReloadBookmark already uses necessary threads internally.

biodranik (Migrated from github.com) commented

2024-08-13 13:43:19 +00:00

@vng is there a check/optimization that if we're already running on the same thread as RunTask is receiving, then the code is run immediately, without putting it into a queue? If no, let's create an issue for it.

map/bookmark_manager.cpp

					
				@ -404,6 +404,53 @@ void BookmarkManager::ResetRecentlyDeletedBookmark()

				  m_recentlyDeletedBookmark.reset();

				}

				size_t BookmarkManager::GetRecentlyDeletedCategoriesCount() const

biodranik (Migrated from github.com) commented

2024-08-13 13:31:15 +00:00

Is this method covered by any test? It should be used by UI to quickly retrieve the count without expensive parsing of all deleted categories.

map/bookmark_manager.cpp

					
				@ -407,0 +411,4 @@

				  return files.size();

				}

				BookmarkManager::KMLDataCollectionPtr BookmarkManager::GetRecentlyDeletedCategories()

biodranik (Migrated from github.com) commented

2024-08-13 13:32:39 +00:00

Is this method called synchronously from the main thread? It may cause a huge delay for large deleted lists in such a case. Please also make sure that it's only called when necessary (when Recently Deleted UI is displayed).

map/bookmark_manager.cpp

					
				@ -407,0 +417,4 @@

				                                  [](kml::FileData const &)

				                                  {

				    return true;

				  });

biodranik (Migrated from github.com) commented

2024-08-13 13:34:04 +00:00

Will this work?

  auto collection = LoadBookmarks(GetTrashDirectory(), kKmlExtension, KmlFileType::Text, [](kml::FileData const &), {});

Will this work? ```suggestion auto collection = LoadBookmarks(GetTrashDirectory(), kKmlExtension, KmlFileType::Text, [](kml::FileData const &), {}); ```

map/bookmark_manager.cpp

					
				@ -407,0 +428,4 @@

				void BookmarkManager::RecoverRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & filePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

biodranik (Migrated from github.com) commented

2024-08-13 13:40:15 +00:00

This check should not be needed as we run the task below on file thread anyway. @vng right?

map/bookmark_manager.cpp

					
				@ -407,0 +442,4 @@

				void BookmarkManager::DeleteRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & deletedFilePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

biodranik (Migrated from github.com) commented

2024-08-13 13:46:05 +00:00

This call may contain a lot of file delete operations. Would it be better to remove the thread checker line and run it on the async File thread instead of the UI thread? It doesn't return anything. But I assume that the caller expects that after returning to the main bookmarks screen files are already deleted, and an updated count in Recently Deleted is displayed. Correct?

map/bookmark_manager.cpp

					
				@ -2499,1 +2543,3 @@

				    FileWriter::DeleteFileX(it->second->GetFileName());

				  auto const & filePath = it->second->GetFileName();

				  if (permanently)

				  {

biodranik (Migrated from github.com) commented

2024-08-13 13:46:59 +00:00

Should it be

  auto const & filePath = it->second->GetFileName();

to avoid unnecessary allocation/copy?

Should it be ```suggestion auto const & filePath = it->second->GetFileName(); ``` to avoid unnecessary allocation/copy?

map/bookmark_manager.hpp

biodranik (Migrated from github.com) commented

2024-08-13 13:49:58 +00:00

    /// @param permanently If true, the file will be removed from the disk. If false, the file will be marked as deleted and moved into a trash.

```suggestion /// @param permanently If true, the file will be removed from the disk. If false, the file will be marked as deleted and moved into a trash. ```

biodranik (Migrated from github.com) commented

2024-08-13 13:50:32 +00:00

Let's require an explicit permanently specification for callers for safety.

    bool DeleteBmCategory(kml::MarkGroupId groupId, bool permanently);

Let's require an explicit `permanently` specification for callers for safety. ```suggestion bool DeleteBmCategory(kml::MarkGroupId groupId, bool permanently); ```

map/bookmark_manager.hpp

biodranik (Migrated from github.com) commented

2024-08-13 13:50:45 +00:00

Can it be const?

biodranik (Migrated from github.com) commented

2024-08-13 13:54:32 +00:00

nit: an alternative approach may leverage something like ForEachDeletedCategory(TLambda &&), but let's leave it for a separate discussion later.

nit: an alternative approach may leverage something like `ForEachDeletedCategory(TLambda &&)`, but let's leave it for a separate discussion later.

NeatNit (Migrated from github.com) reviewed 2024-08-13 14:14:04 +00:00

data/strings/strings.txt

NeatNit (Migrated from github.com) commented

2024-08-13 14:14:04 +00:00

he is good.

`he` is good.

Ghost reviewed 2024-08-13 15:00:25 +00:00

data/strings/strings.txt

Ghost commented

2024-08-13 15:00:25 +00:00

Look good for French ^^

rtsisyk reviewed 2024-08-14 14:17:17 +00:00

map/bookmark_manager.cpp

					
				@ -2499,1 +2543,3 @@

				    FileWriter::DeleteFileX(it->second->GetFileName());

				  auto const & filePath = it->second->GetFileName();

				  if (permanently)

				  {

rtsisyk commented

2024-08-14 14:17:16 +00:00

How much $ does it save?

How much $$$ does it save?

rtsisyk reviewed 2024-08-14 14:18:03 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +442,4 @@

				void BookmarkManager::DeleteRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & deletedFilePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

rtsisyk commented

2024-08-14 14:18:03 +00:00

As far as I can see, all deletion operations in the existing code are currently performed in the UI thread.

kirylkaveryn reviewed 2024-08-14 14:27:38 +00:00

map/bookmark_manager.cpp

					
				@ -404,6 +404,53 @@ void BookmarkManager::ResetRecentlyDeletedBookmark()

				  m_recentlyDeletedBookmark.reset();

				}

				size_t BookmarkManager::GetRecentlyDeletedCategoriesCount() const

kirylkaveryn commented

2024-08-14 14:27:38 +00:00

I fixed the current cpp test and added checks for the GetRecentlyDeletedCategoriesCount

kirylkaveryn reviewed 2024-08-14 14:30:47 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +417,4 @@

				                                  [](kml::FileData const &)

				                                  {

				    return true;

				  });

kirylkaveryn commented

2024-08-14 14:30:47 +00:00

No. The method was designed to work with the using BookmarksChecker = std::function<bool(kml::FileData const &)>;
that should returns bool:

No. The method was designed to work with the ` using BookmarksChecker = std::function<bool(kml::FileData const &)>;` that should returns bool: <img width="1183" alt="image" src="https://github.com/user-attachments/assets/17737cb6-ffcc-4f27-b2e5-e2ac5a5c0e42">

kirylkaveryn reviewed 2024-08-14 14:35:42 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +411,4 @@

				  return files.size();

				}

				BookmarkManager::KMLDataCollectionPtr BookmarkManager::GetRecentlyDeletedCategories()

kirylkaveryn commented

2024-08-14 14:35:42 +00:00

Is this method called synchronously from the main thread?

Yes

It may cause a huge delay for large deleted lists in such a case. Please also make sure that it's only called when necessary (when Recently Deleted UI is displayed).

This is why the GetRecentlyDeletedCategoriesCount was added.

The GetRecentlyDeletedCategories was called only when the RecentlyDeletedCategoriesViewController is opening and during the it's updates (when the category was deleted by the cloud for example).

> Is this method called synchronously from the main thread? Yes > It may cause a huge delay for large deleted lists in such a case. Please also make sure that it's only called when necessary (when Recently Deleted UI is displayed). This is why the `GetRecentlyDeletedCategoriesCount` was added. The `GetRecentlyDeletedCategories` was called only when the `RecentlyDeletedCategoriesViewController` is opening and during the it's updates (when the category was deleted by the cloud for example).

kirylkaveryn reviewed 2024-08-14 14:43:55 +00:00

map/bookmark_manager.cpp

kirylkaveryn commented

2024-08-14 14:43:55 +00:00

@biodranik the Reload and Load methods check the thread first so they should be called from that main (maybe because to work safe with the m_bookmarkLoadingQueue)

void BookmarkManager::LoadBookmark(std::string const & filePath, bool isTemporaryFile)
{
  CHECK_THREAD_CHECKER(m_threadChecker, ());
  // Defer bookmark loading in case of another asynchronous process.
  if (!m_loadBookmarksFinished || m_asyncLoadingInProgress)
  {
    m_bookmarkLoadingQueue.emplace_back(filePath, isTemporaryFile, false);
    return;
  }

  NotifyAboutStartAsyncLoading();
  LoadBookmarkRoutine(filePath, isTemporaryFile);
}

void BookmarkManager::ReloadBookmark(std::string const & filePath)
{
  CHECK_THREAD_CHECKER(m_threadChecker, ());
  if (!m_loadBookmarksFinished || m_asyncLoadingInProgress)
  {
    m_bookmarkLoadingQueue.emplace_back(filePath, false, true);
    return;
  }
  NotifyAboutStartAsyncLoading();
  ReloadBookmarkRoutine(filePath);
}

@biodranik the Reload and Load methods check the thread first so they should be called from that main (maybe because to work safe with the m_bookmarkLoadingQueue) ```cpp void BookmarkManager::LoadBookmark(std::string const & filePath, bool isTemporaryFile) { CHECK_THREAD_CHECKER(m_threadChecker, ()); // Defer bookmark loading in case of another asynchronous process. if (!m_loadBookmarksFinished || m_asyncLoadingInProgress) { m_bookmarkLoadingQueue.emplace_back(filePath, isTemporaryFile, false); return; } NotifyAboutStartAsyncLoading(); LoadBookmarkRoutine(filePath, isTemporaryFile); } void BookmarkManager::ReloadBookmark(std::string const & filePath) { CHECK_THREAD_CHECKER(m_threadChecker, ()); if (!m_loadBookmarksFinished || m_asyncLoadingInProgress) { m_bookmarkLoadingQueue.emplace_back(filePath, false, true); return; } NotifyAboutStartAsyncLoading(); ReloadBookmarkRoutine(filePath); } ```

kirylkaveryn reviewed 2024-08-14 14:53:06 +00:00

map/bookmark_manager.hpp

kirylkaveryn commented

2024-08-14 14:53:06 +00:00

Can it be const?

No, because the LoadBookmarks cannot.

> Can it be const? No, because the LoadBookmarks cannot.

kirylkaveryn reviewed 2024-08-14 15:16:52 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +442,4 @@

				void BookmarkManager::DeleteRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & deletedFilePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

kirylkaveryn commented

2024-08-14 15:16:52 +00:00

The main idea is to call this method synchronously on the main and Not pass the completion handler -> update the state immediately without waiting for deletion ansync finish.
Deletion is quite fast and it is better to rely on the actual directory content.

For example:

user press delete all
deletion task is dispatched to the file thread
the screen is closed
the code checks the trash directory and may see that not all the files is deleted - it will cause a bug because the GetRecentlyDeletedCategoriesCount returns > 0

The main idea is to call this method synchronously on the main and Not pass the completion handler -> update the state immediately without waiting for deletion ansync finish. Deletion is quite fast and it is better to rely on the actual directory content. For example: 1. user press `delete all` 2. deletion task is dispatched to the file thread 3. the screen is closed 4. the code checks the trash directory and may see that not all the files is deleted - it will cause a bug because the GetRecentlyDeletedCategoriesCount returns > 0

kirylkaveryn reviewed 2024-08-14 15:35:13 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +428,4 @@

				void BookmarkManager::RecoverRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & filePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

kirylkaveryn commented

2024-08-14 15:35:12 +00:00

Thanks!

kirylkaveryn reviewed 2024-08-14 15:38:00 +00:00

map/bookmark_manager.cpp

kirylkaveryn commented

2024-08-14 15:38:00 +00:00

Thanks! Semantically it is not the scanning the folder but it works as expected (and faster).

kirylkaveryn reviewed 2024-08-14 15:39:30 +00:00

map/bookmark_manager.hpp

kirylkaveryn commented

2024-08-14 15:39:30 +00:00

This will require changing the Android's code...

kirylkaveryn reviewed 2024-08-14 15:50:02 +00:00

data/strings/strings.txt

kirylkaveryn commented

2024-08-14 15:50:02 +00:00

It seems like all translations is correct

rtsisyk reviewed 2024-08-14 16:17:12 +00:00

map/bookmark_manager.hpp

rtsisyk commented

2024-08-14 16:17:12 +00:00

Let's require an explicit permanently specification for callers for safety.

Could you please provide more details on why you believe this change is necessary?

> Let's require an explicit `permanently` specification for callers for safety. Could you please provide more details on why you believe this change is necessary?

rtsisyk reviewed 2024-08-14 16:18:39 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +442,4 @@

				void BookmarkManager::DeleteRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & deletedFilePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

rtsisyk commented

2024-08-14 16:18:39 +00:00

Enhancing the error handling for I/O operations in the Storage thread across the code base is a good task to be tackled separately. This PR follows the existing approach.

kirylkaveryn reviewed 2024-08-14 16:36:45 +00:00

map/bookmark_manager.hpp

kirylkaveryn commented

2024-08-14 16:36:45 +00:00

Fixed!

biodranik (Migrated from github.com) reviewed 2024-08-14 18:23:52 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +442,4 @@

				void BookmarkManager::DeleteRecentlyDeletedCategoriesAtPaths(std::vector<std::string> const & deletedFilePaths)

				{

				  CHECK_THREAD_CHECKER(m_threadChecker, ());

biodranik (Migrated from github.com) commented

2024-08-14 18:23:52 +00:00

I understand the reasoning for the synchronous call.

Won't the android linter/runtime checker complain for file operations made on the UI thread in this case?

I understand the reasoning for the synchronous call. Won't the android linter/runtime checker complain for file operations made on the UI thread in this case?

biodranik (Migrated from github.com) reviewed 2024-08-14 18:30:00 +00:00

map/bookmark_manager.cpp

					
				@ -407,0 +417,4 @@

				                                  [](kml::FileData const &)

				                                  {

				    return true;

				  });

biodranik (Migrated from github.com) commented

2024-08-14 18:29:59 +00:00

Sorry for the typo, of course I meant an empty std::function/lambda, which compiles:
auto collection = LoadBookmarks(GetTrashDirectory(), kKmlExtension, KmlFileType::Text, {});

Sorry for the typo, of course I meant an empty std::function/lambda, which compiles: `auto collection = LoadBookmarks(GetTrashDirectory(), kKmlExtension, KmlFileType::Text, {});`

biodranik (Migrated from github.com) reviewed 2024-08-14 18:37:02 +00:00

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2024-08-14 18:37:01 +00:00

This approach looks like a heavy overhead:

Run on File thread
Move file
Run on GUI/main thread
Reload bookmarks
ReloadBookmarkRoutine
Run on File thread
Notify on GUI thread

If it's ok to delete files from the main thread, then it should be also ok to move files from the main thread. RecoverRecentlyDeletedCategoriesAtPaths is called from the main thread, right? Can step (1) and (3) be removed in this case?

This approach looks like a heavy overhead: 1. Run on File thread 2. Move file 3. Run on GUI/main thread 4. Reload bookmarks 5. ReloadBookmarkRoutine 6. Run on File thread 7. Notify on GUI thread If it's ok to delete files from the main thread, then it should be also ok to move files from the main thread. `RecoverRecentlyDeletedCategoriesAtPaths` is called from the main thread, right? Can step (1) and (3) be removed in this case?

biodranik (Migrated from github.com) reviewed 2024-08-14 18:38:13 +00:00

map/bookmark_manager.cpp

biodranik (Migrated from github.com) commented

2024-08-14 18:38:13 +00:00

nit: spaces are inserted automatically in our logging/assert macros.

    CHECK(IsRecentlyDeletedCategory(deletedFilePath), ("The category at path", deletedFilePath, "should be in the trash."));

nit: spaces are inserted automatically in our logging/assert macros. ```suggestion CHECK(IsRecentlyDeletedCategory(deletedFilePath), ("The category at path", deletedFilePath, "should be in the trash.")); ```

biodranik (Migrated from github.com) reviewed 2024-08-14 18:40:54 +00:00

map/map_tests/bookmarks_test.cpp

biodranik (Migrated from github.com) commented

2024-08-14 18:40:54 +00:00

  TEST_EQUAL(bmManager.GetBmGroupsCount(), 1, ());
  TEST_EQUAL(bmManager.GetRecentlyDeletedCategoriesCount(), 0, ());

```suggestion TEST_EQUAL(bmManager.GetBmGroupsCount(), 1, ()); TEST_EQUAL(bmManager.GetRecentlyDeletedCategoriesCount(), 0, ()); ```

biodranik (Migrated from github.com) reviewed 2024-08-14 18:44:15 +00:00

map/bookmark_manager.cpp

					
				@ -2497,3 +2542,2 @@

				  if (deleteFile)

				    FileWriter::DeleteFileX(it->second->GetFileName());

				  auto const & filePath = it->second->GetFileName();

biodranik (Migrated from github.com) commented

2024-08-14 18:44:15 +00:00

    if (base::MoveFileX(filePath, trashedFilePath))
      LOG(LINFO, ("Category at", filePath, "is trashed to the", trashedFilePath));
    else
      LOG(LERROR, ("Failed to move", filePath, "into the trash at", trashedFilePath));

```suggestion if (base::MoveFileX(filePath, trashedFilePath)) LOG(LINFO, ("Category at", filePath, "is trashed to the", trashedFilePath)); else LOG(LERROR, ("Failed to move", filePath, "into the trash at", trashedFilePath)); ```

biodranik (Migrated from github.com) reviewed 2024-08-14 18:50:46 +00:00

iphone/CoreApi/CoreApi/Bookmarks/RecentlyDeletedCategory/RecentlyDeletedCategory.mm

biodranik (Migrated from github.com) commented

2024-08-14 18:50:46 +00:00

nit: avoid unnecessary copy

- (instancetype)initWithCategoryData:(kml::CategoryData)data filePath:(std::string const &)filePath {

nit: avoid unnecessary copy ```suggestion - (instancetype)initWithCategoryData:(kml::CategoryData)data filePath:(std::string const &)filePath { ```

biodranik (Migrated from github.com) reviewed 2024-08-14 19:15:20 +00:00

map/bookmark_manager.cpp

					
				@ -2499,1 +2543,3 @@

				    FileWriter::DeleteFileX(it->second->GetFileName());

				  auto const & filePath = it->second->GetFileName();

				  if (permanently)

				  {

biodranik (Migrated from github.com) commented

2024-08-14 19:15:20 +00:00

It's not about $. It's about clarity and clear code. And understanding what's going on.

It's not about $$$. It's about clarity and clear code. And understanding what's going on.

biodranik (Migrated from github.com) reviewed 2024-08-14 19:22:00 +00:00

map/bookmark_manager.hpp

biodranik (Migrated from github.com) commented

2024-08-14 19:22:00 +00:00

Actually, LoadBookmarks method should be marked as const, because it doesn't modify any members. Then GetRecentlyDeletedCategories() can also be properly marked as const. Otherwise, reading it, there is an assumption that it changes the internal state, while it's not.