FR: Storage - use NSProgress objects properly when reporting progress, not re-initting them every time

[xaphod]

[READ] Guidelines

Feature proposal

• Firebase Component: Storage

In FIRStorageTask.m there's this code for reporting progress:

- (FIRStorageTaskSnapshot *)snapshot {
  @synchronized(self) {
    NSProgress *progress = [NSProgress progressWithTotalUnitCount:self.progress.totalUnitCount];

A new NSProgress object is created every time there's a snapshot. This makes it impossible to use NSProgress objects how they are often used: they get added as a child of another NSProgress object, via NSProgress.addChild(...). That's because often one has lots of download tasks and wants to report overall/aggregate progress to the user -- and rather than having to calculate this every time something changes, one just observes a parent NSProgress object.

Ask: instead of making a new NSProgress object every time, please reuse one NSProgress object per task, specifically so we can add it as a child of a "parent" NSProgress object.

Bonus ask, worth 10,000 points: also support NSProgress' pause/unpause/cancel functionality, so we can easily tie in pausing multiple tasks at once

Thanks!

[Dionne-Rodriguez]

spent hours trying to implement my own logic to handle multiple image uploads, but it is very hard to track the progress of each unique upload because NSProgress gets recreated everytime. Hopefully the firebase devs can fix this issue swiftly 🧐

[paulb777]

Sorry about the trouble. We're happy to review a PR that addresses this. Otherwise, we'll see if we can include better NSProgress support in this year's Swift API improvement initiative.

[xaphod]

@Dionne-Rodriguez Here's how I handled it in case it helps. I'm in a rush so if you have questions just ask.
Key learning: use the magical Progress.current().
A Download is a struct containing a Progress.
The one Progress to rule them all (and in the darkness bind them) is self.progress, updated in updateRootProgress()

            var download = Download.init(storagePath: storagePath)
            download.progress = Progress.init(parent: Progress.current(), userInfo: nil)
            let path = subfolder + "/" + String(localFilename)
            download.pathRelativeToFolder = path
            self.downloads[storagePath] = download
            let fullPath = self.folderUrl.appendingPathComponent(path)
            self.printDebugProgressInfo(for: download)

            let storageRef = Storage.storage().reference(withPath: storagePath)
            // get the metadata up front so we see the total before the download finishes for small files
            storageRef.getMetadata { (meta, error) in
                if let meta = meta {
                    self.downloads[storagePath]?.progress.totalUnitCount = meta.size
                    self.updateRootProgress()
                }
                let task = storageRef.write(toFile: fullPath, forbidBackgroundSessions: true, completion: { (u, e) in self.didDownload(storagePath: storagePath, url: u, error: e) })
                let _ = task.observe(.progress) { (snapshot) in
                    if let p = snapshot.progress {
                        self.downloads[storagePath]?.progress.totalUnitCount = p.totalUnitCount
                        self.downloads[storagePath]?.progress.completedUnitCount = p.completedUnitCount
                    }
                    self.updateRootProgress()
                    self.printDebugProgressInfo(for: download)
                }
            }

    fileprivate func updateRootProgress() {
        let progresses = self.downloads.values.compactMap { $0.progress }
        self.progress.totalUnitCount = progresses.reduce(0, { $0 + $1.totalUnitCount })
        self.progress.completedUnitCount = progresses.reduce(0, { $0 + $1.completedUnitCount })
        DispatchQueue.main.async {
            NotificationCenter.default.post(name: .downloadProgress, object: nil)
        }
    }

[paulb777]

Some progress on this feature request in #10161

[paulb777]

It seems that the current implementation is designed to be an immutable snapshot of the progress.

Would it be a solution to change the Progress associated with the StorageTask from internal to public? https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorage/Sources/StorageTask.swift#L67