[ios] Base implementation of iCloud Synchronization #7641

kirylkaveryn · 2024-03-20T14:10:32Z

kirylkaveryn commented

2024-03-20 14:10:32 +00:00

This PR adds multi-device synchronization of bookmarks to iCloud on iOS:

Bookmarks are synchronized across Apple devices by using iCloud.
Conflicts are resolved automatically by duplicating affected bookmarks.
New KML files with bookmarks can be added via Files App (iOS) or via Finder (MacOS).
Deleted bookmark files can be restored via Recently deleted (iOS) or in Bin (MacOS).

Closes #2678

Demos

Screen recordings with 2 devices and overall workflow you can see by the link (because they are even compressed bigger than 10mb):
https://drive.google.com/drive/folders/1K9aqaElsh6YOFps8dg1pJ_dK2XoBVlgB?usp=drive_link

Create bookmarks
Create a new list
Delete list from the file system
Drag and drop file into the iCloud container
Restore recentlky deleted file from Bin (onMac)
Merge conflict when on the both devices lists was changed but the one device was offline
Add bookmarks whe device is offline
Exportl kml file from GuruMaps directly into the Files app iCloud container

To Do

Things that are still in progress and should be discussed. Please share your thoughts on how to implement them!

There are many places in this implementation that may raise errors (starting from metadata items initialization, read/write operations, and continuing with the quota error: NSUbiquitousFileNotUploadedDueToQuotaError).
It is an open question about how to handle errors.

NSUbiquitousFileNotUploadedDueToQuotaError doesn't interrupt the writing into the iCloud local container, so this error can be handled as usual.
Fixed by displaying the sync error on the Settings screen
UI For handling the Errors and uploading process in the Bookmarks Table View Cell (as in the Files app)
UI for enable/disable Sync. I'll make an additional switcher in the settings which will be Off by default. And I want to add an Alert that informs users that the feature is in Beta and that it's better to back up all bookmarks for safety reasons.
Handle disabled iCloud on the Device in Settings.
Now the reloading of bookmarks when all files were updated/saved starts by calling the bookmarksManager.loadBookmarks() which reloads All files. But it would be better to reload only provided groups. It needs to implement a method that will process only the current file and notify when all work is finished.
When the app is installed for the first time and there are no bookmarks at all it means that we shouldn't process a file called My Places.kml and start downloading the cloud content (if needed) but there are no methods in the core that can return some BookmarksManager.userContentIsEmpy bool because there already exists one file. So for the initial state, I use the hard coding to determine that there is really NO content. It needs to add some more convenient ways to handle situations when there is no content.

The new isEmpty method was added to the Bookmarks Manager.
Add the C617.1 API Declaration "Declare this reason to access the timestamps, size, or other metadata of files inside the app container, app group container, or the app’s CloudKit container." to the PrivacyInfo
Unit tests
- SynchronizationStateManager and LocalDirectoryMonitor are partly covered
Bug:
Sometimes, lists are duplicated in the table (not the files themselves, but the lists) on the Bookmarks and Tracks screen. This is somehow related to the core and the generation of bookmarks, as the table draws what is generated by the core. Apparently, if loadBookmarks is called frequently or several times in succession, the cleaning is not performed correctly.
Found the reason: If the loadBookmarks method is called twice in a row, the lists will duplicate even without the cloud (in the master).
Bug: Fix the infinity loop on deletion of different bookmarks in the same group on different devices in parallel.
Bug: Fix UI crashes when the same file is changed on two devices in parallel
- Reload only changed files instead of reloading all files by using LoadBookmarks().
- Don't change GroupMarkId when reloading the file with the same filename.
- Invalidate UI by GroupMarkId when Group/File has been changed.

Nice to Have

Optimization: Consider storing the visibility status of groups (the 'eye' icon) outside of KML files to reduce overhead and minimize conflicts during synchronization (especially actual when the user presses "show/hide all").
Optimization: Investigate why timestamps are periodically updated for all files without any changes.

Further Improvements (out of scope of this PR)

Feature: Ability to parse kmz/gpx files that were placed by a user directly into the iCloud local container on the device
Feature: Sync the iCloud enabling setting across devices using NSUbiquitousKeyValueStore
Feature: Implement backup of nested folders and files. The current implementation works only with the kmls files. But in the future, there will be user-generated content such as images. So it is important to move from using fielNames as a uuid to the related paths.
Feature: Handling of the unsupported/corrupted files to notify a user (at least an alert with a message that "file is not supported")
Feature: Show the Toast message when the PlacePage or the Edit screen were dismissed during the MarkID invalidation.
Replace the "iCloud Synchronization (Beta)" string withe the clearer explanation
Add a hint to attract user to enable feature

This PR adds multi-device synchronization of bookmarks to iCloud on iOS: - Bookmarks are synchronized across Apple devices by using iCloud. - Conflicts are resolved automatically by duplicating affected bookmarks. - New KML files with bookmarks can be added via `Files App` (iOS) or via `Finder` (MacOS). - Deleted bookmark files can be restored via `Recently deleted` (iOS) or in `Bin` (MacOS). Closes #2678 ### Demos Screen recordings with 2 devices and overall workflow you can see by the link (because they are even compressed bigger than 10mb): https://drive.google.com/drive/folders/1K9aqaElsh6YOFps8dg1pJ_dK2XoBVlgB?usp=drive_link [Create bookmarks](https://drive.google.com/file/d/1q6ZyykJjXHdOBkWdLDLeI1uOqzs-6Hu9/view?usp=drive_link) [Create a new list](https://drive.google.com/file/d/1Nzi0oS449kveMML4p8pb_i8OhXEha67R/view?usp=drive_link) [Delete list from the file system](https://drive.google.com/file/d/1LtSu_Ya9bDLUgzyqOf9e5ok7AxYw0Rxx/view?usp=drive_link) [Drag and drop file into the iCloud container](https://drive.google.com/file/d/1KcigdSH9R3pkDvLPjm8MTMv_SLqhgU3t/view?usp=drive_link) [Restore recentlky deleted file from Bin (onMac)](https://drive.google.com/file/d/1HkqExuHv3MoW-Mjog-ufdMPY3zKuUoRv/view?usp=drive_link) [Merge conflict when on the both devices lists was changed but the one device was offline](https://drive.google.com/file/d/1u1KfSgFuJCdXVGnguxVj8sPBbFIW3oQo/view?usp=drive_link) [Add bookmarks whe device is offline](https://drive.google.com/file/d/1nYWPmtVcS0986aNY4paA898sHG0xModx/view?usp=drive_link) [Exportl kml file from GuruMaps directly into the Files app iCloud container](https://drive.google.com/file/d/1_NhAXdCDKYiRwunCE8ag0lMJLSxlp8iK/view?usp=drive_link) ------------------------------------------------------------- ### To Do Things that are still in progress and should be discussed. Please share your thoughts on how to implement them! - [x] There are many places in this implementation that may raise errors (starting from metadata items initialization, read/write operations, and continuing with the quota error: NSUbiquitousFileNotUploadedDueToQuotaError). It is an open question about how to handle errors. > `NSUbiquitousFileNotUploadedDueToQuotaError` doesn't interrupt the writing into the iCloud local container, so this error can be handled as usual. > Fixed by displaying the sync error on the Settings screen - [x] UI For handling the Errors and uploading process in the Bookmarks Table View Cell (as in the Files app) <img width="329" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/9a78c977-a217-4e58-a246-7e9907149801"> - [x] UI for enable/disable Sync. I'll make an additional switcher in the settings which will be Off by default. And I want to add an Alert that informs users that the feature is in Beta and that it's better to back up all bookmarks for safety reasons. - [x] Handle `disabled iCloud on the Device` in Settings. - [x] Now the reloading of bookmarks when all files were updated/saved starts by calling the `bookmarksManager.loadBookmarks()` which reloads All files. But it would be better to reload only provided groups. It needs to implement a method that will process only the current file and notify when all work is finished. - [x] When the app is installed for the first time and there are no bookmarks at all it means that we shouldn't process a file called `My Places.kml` and start downloading the cloud content (if needed) but there are no methods in the core that can return some `BookmarksManager.userContentIsEmpy` bool because there already exists one file. So for the initial state, I use the hard coding to determine that there is really NO content. It needs to add some more convenient ways to handle situations when there is `no content`. > The new `isEmpty` method was added to the `Bookmarks Manager`. - [x] Add the C617.1 API Declaration "Declare this reason to access the timestamps, size, or other metadata of files inside the app container, app group container, or the app’s CloudKit container." to the PrivacyInfo - [ ] Unit tests - `SynchronizationStateManager` and `LocalDirectoryMonitor` are partly covered - [x] Bug: Sometimes, lists are duplicated in the table (not the files themselves, but the lists) on the Bookmarks and Tracks screen. This is somehow related to the core and the generation of bookmarks, as the table draws what is generated by the core. Apparently, if loadBookmarks is called frequently or several times in succession, the cleaning is not performed correctly. Found the reason: If the loadBookmarks method is called twice in a row, the lists will duplicate even without the cloud (in the master). - [x] Bug: Fix the infinity loop on deletion of different bookmarks in the same group on different devices in parallel. - [x] Bug: Fix UI crashes when the same file is changed on two devices in parallel - [x] Reload only **changed** files instead of reloading all files by using LoadBookmarks(). - [x] Don't change GroupMarkId when reloading the file with the same filename. - [x] Invalidate UI by GroupMarkId when Group/File has been changed. ### Nice to Have - [ ] Optimization: Consider storing the visibility status of groups (the 'eye' icon) outside of KML files to reduce overhead and minimize conflicts during synchronization (especially actual when the user presses "show/hide all"). - [ ] Optimization: Investigate why timestamps are periodically updated for all files without any changes. ### Further Improvements (out of scope of this PR) - [ ] Feature: Ability to parse kmz/gpx files that were placed by a user directly into the iCloud local container on the device - [ ] Feature: Sync the iCloud enabling setting across devices using `NSUbiquitousKeyValueStore` - [ ] Feature: Implement backup of nested folders and files. The current implementation works only with the `kml`s files. But in the future, there will be user-generated content such as images. So it is important to move from using `fielName`s as a uuid to the related paths. - [ ] Feature: Handling of the unsupported/corrupted files to notify a user (at least an alert with a message that "file is not supported") - [ ] Feature: Show the `Toast message` when the `PlacePage` or the `Edit screen` were dismissed during the `MarkID` invalidation. - [ ] Replace the "iCloud Synchronization (Beta)" string withe the clearer explanation - [ ] Add a hint to attract user to enable feature

🎉 13

biodranik (Migrated from github.com) reviewed 2024-04-30 21:53:07 +00:00

iphone/Maps/PrivacyInfo.xcprivacy

biodranik (Migrated from github.com) commented

2024-04-30 21:53:07 +00:00

This should go to AccessedAPICategoryFileTimestamp, right?

This should go to `AccessedAPICategoryFileTimestamp`, right?

kirylkaveryn reviewed 2024-05-01 05:17:35 +00:00

iphone/Maps/PrivacyInfo.xcprivacy

kirylkaveryn commented

2024-05-01 05:17:35 +00:00

Thanks! This code is set incorrectly here. I'll fix it. This is for the timestamp

biodranik (Migrated from github.com) reviewed 2024-05-01 20:17:12 +00:00

iphone/Maps/Core/iCloud/DefaultLocalDirectoryMonitor.swift

					
				@ -0,0 +192,4 @@

				private extension FileManager {

				  func source(for directory: URL) throws -> DispatchSourceFileSystemObject {

				    let directoryFileDescriptor = open(directory.path, O_EVTONLY)

biodranik (Migrated from github.com) commented

2024-05-01 20:17:12 +00:00

This code monitor only directory changes, not file changes. E.g. if you edit/overwrite some file in-place, it won't fire. If you add a new file, delete, change file name, then it fires.

Our bookmarks saving code should create a new temporary file, and move it to overwrite the old file atomically, that's why it works.

However, there could be some other cases when files are just overwritten/appended, without modifying the directory.

Another case to test: what if user uploads some good/bad kml files (and other types too) using Finder/File Sharing into OM's monitored folder?

This code monitor only directory changes, not file changes. E.g. if you edit/overwrite some file in-place, it won't fire. If you add a new file, delete, change file name, then it fires. Our bookmarks saving code should create a new temporary file, and move it to overwrite the old file atomically, that's why it works. However, there could be some other cases when files are just overwritten/appended, without modifying the directory. Another case to test: what if user uploads some good/bad kml files (and other types too) using Finder/File Sharing into OM's monitored folder?

kirylkaveryn reviewed 2024-05-02 07:05:04 +00:00

iphone/Maps/Core/iCloud/DefaultLocalDirectoryMonitor.swift

					
				@ -0,0 +192,4 @@

				private extension FileManager {

				  func source(for directory: URL) throws -> DispatchSourceFileSystemObject {

				    let directoryFileDescriptor = open(directory.path, O_EVTONLY)

kirylkaveryn commented

2024-05-02 07:05:04 +00:00

This code is triggered on every write operation into the directory, not only the creation/renaming/removal because I've set up the source with the write eventMask:
https://developer.apple.com/documentation/dispatch/dispatchsource/filesystemevent/1780646-write

let dispatchSource = DispatchSource.makeFileSystemObjectSource(fileDescriptor: directoryFileDescriptor, eventMask: [.write], queue: DispatchQueue.main)

I wrote the test case to check the file contents updating without replacing isolated from the bookmark manager:

Another case to test: what if user uploads some good/bad kml files (and other types too) using Finder/File Sharing into OM's monitored folder?

We can only add files to the iCloud local container.

Good kmls will be parsed and displayed immediately
Bad kmls will not be displayed because they will not be parsed - I mentioned in todo that we should add an alert to handle this situation and notify a user.
Other files (not *.kml) will not be synced (until we implement the support of the nested files/directories).

This code is triggered on _every write operation_ into the directory, not only the creation/renaming/removal because I've set up the source with the `write` eventMask: https://developer.apple.com/documentation/dispatch/dispatchsource/filesystemevent/1780646-write ```swift let dispatchSource = DispatchSource.makeFileSystemObjectSource(fileDescriptor: directoryFileDescriptor, eventMask: [.write], queue: DispatchQueue.main) ``` I wrote the test case to check the file contents _updating without replacing_ isolated from the bookmark manager: <img width="806" alt="image" src="https://github.com/organicmaps/organicmaps/assets/79797627/feef7adb-0639-4fd8-940c-0175e11e365d"> > Another case to test: what if user uploads some good/bad kml files (and other types too) using Finder/File Sharing into OM's monitored folder? We can only add files to the iCloud local container. - Good kmls will be parsed and displayed immediately - Bad kmls will not be displayed because they will not be parsed - I mentioned in todo that we should add an alert to handle this situation and notify a user. - Other files (not *.kml) will not be synced (until we implement the support of the nested files/directories).

biodranik (Migrated from github.com) reviewed 2024-05-02 07:48:23 +00:00

iphone/Maps/Core/iCloud/DefaultLocalDirectoryMonitor.swift

					
				@ -0,0 +192,4 @@

				private extension FileManager {

				  func source(for directory: URL) throws -> DispatchSourceFileSystemObject {

				    let directoryFileDescriptor = open(directory.path, O_EVTONLY)

biodranik (Migrated from github.com) commented

2024-05-02 07:48:23 +00:00

Is any appending write to the existing file also catched? Can a test for append write be added?

Is it possible to avoid detection of appending data?

My Files app shows non-kml files. Is it a bug that they are synced to iCloud?

Is any appending write to the existing file also catched? Can a test for append write be added? Is it possible to avoid detection of appending data? My Files app shows non-kml files. Is it a bug that they are synced to iCloud? ![image](https://github.com/organicmaps/organicmaps/assets/170263/db5055d0-ffd6-4f88-9d8a-91ac5880eb9d)

kirylkaveryn reviewed 2024-05-02 07:55:20 +00:00

iphone/Maps/Core/iCloud/DefaultLocalDirectoryMonitor.swift

					
				@ -0,0 +192,4 @@

				private extension FileManager {

				  func source(for directory: URL) throws -> DispatchSourceFileSystemObject {

				    let directoryFileDescriptor = open(directory.path, O_EVTONLY)

kirylkaveryn commented

2024-05-02 07:55:20 +00:00

My Files app shows non-kml files. Is it a bug that they are synced to iCloud?

It seems that these are temporary kmls created by the core...
I'll add filtering to these files.

> My Files app shows non-kml files. Is it a bug that they are synced to iCloud? It seems that these are temporary kmls created by the core... I'll add filtering to these files.

biodranik (Migrated from github.com) reviewed 2024-05-02 16:00:21 +00:00

iphone/Maps/Core/iCloud/DefaultLocalDirectoryMonitor.swift

					
				@ -0,0 +192,4 @@

				private extension FileManager {

				  func source(for directory: URL) throws -> DispatchSourceFileSystemObject {

				    let directoryFileDescriptor = open(directory.path, O_EVTONLY)

biodranik (Migrated from github.com) commented

2024-05-02 16:00:20 +00:00

Another question is why these temporary files are not deleted...

rtsisyk approved these changes 2024-05-24 09:31:19 +00:00

rtsisyk left a comment

It works like a charm in the latest version (32d9e014bc99202759f4b1d026745e50b38d2b01). Tested on two devices. No crashes. No data lost. Very interactive. Great work!

fabwu (Migrated from github.com) approved these changes 2024-05-27 15:07:04 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

rtsisyk

fabwu