digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 1 forks source link

fix: simplify technical implementation of importing custom map file #855

Closed achou11 closed 3 days ago

achou11 commented 3 days ago

The previous approach:

  1. Pick a map file via the file picker, which copies the file to the app's cache storage.
  2. If valid, move the file from the cache storage into the app's document storage.

Both steps have a performance overhead that is most noticeable when working with large files. The overhead in (1) is something we've run into before. The issue in (2) is more theoretical, with the suspicion that potentially expensive deletions need to happen internally when moving a file from one storage directory.

This PR changes the approach to the following:

  1. Pick a map file via the file picker, which no longer copies it into the cache storage.
  2. Copy the selected file into the app document storage using the SAF uri, which represents the original location of the selected file.

This should have no user-facing impact, aside from potential performance improvements due to doing less work internally.

gmaclennan commented 3 days ago

I would just drop the opts.copyToCache option. It's not being used yet, and it's probably not a great idea in most circumstances. You can add it if needed in the future, although probably in most cases it's going to be better to copy after getting a return value from the document picker, so that you can show better user feedback.

gmaclennan commented 3 days ago

I would just drop the opts.copyToCache option. It's not being used yet, and it's probably not a great idea in most circumstances. You can add it if needed in the future, although probably in most cases it's going to be better to copy after getting a return value from the document picker, so that you can show better user feedback.

e.g. always set it to false and don't expose it as an option for useSelectFile().

achou11 commented 3 days ago

I would just drop the opts.copyToCache option. It's not being used yet, and it's probably not a great idea in most circumstances. You can add it if needed in the future, although probably in most cases it's going to be better to copy after getting a return value from the document picker, so that you can show better user feedback.

yeah that's valid. had kept it in there because the result of DocumentPicker.getAsync() provides more useful information if copying to the cache directory, at least based on my memory of how we used it in Mapeo Mobile.

achou11 commented 3 days ago

@gmaclennan I implemented your suggestion via b1db178accbe10e08e7ebc9c2c9ffa2536e8603d