Handle lookup item & attachment creation in IdentifierLookupController

[mvasilak]

closes #683

Moves item & attachment creation from identifier lookup to a separate user controller, that is guarranted to fulfill ongoing lookups, if lookup window is closed.

With this change, perhaps we should label the button as Close, instead of Cancel. Furthermore, the items added for this identifier, perhaps should indicate that they might have pending downloads.

--

TODO:

• Keep track of separate lookup window initializations, if selected library & collections are different.
• Include downloaded attachments from translated identifiers, in items toolbar label Downloaded x/y.
• Show progress of identifier lookup in items toolbar label as Saved x/y.
• Add button in items toolbar, that will restore lookup window, when there are identifier lookups in the background.
• Add Cancel all button in lookup window, that will cancel all downloads and trash items/attachments already fetched.
• Rename current Cancel button in lookup window to Close, that dismisses window and continues lookup in the background.
• Add a cancel button in each lookup item row, for individual cancellation. Cancelling an item, will also remove its attachments. Use an X or trash icon.
• Add a cancel button in each attachment row, that cancels/removes this attachment.
• Cleanup of finished lookup items from controller, when lookup window is closed, and all operations are finished/cancelled.
• When new identifiers are added for lookup, with previous ones queued, show a list of all items.

[michalrentka]

Code-wise it's fine, it moved existing functionality to background so that things can finish even if the UI is closed.

With this change, perhaps we should label the button as Close, instead of Cancel

@mvasilak I think that Cancel currently still cancels the translation? The web view is still inside the popup, so if the popup closes, translation is cancelled, right? I'd keep it as it is - you can cancel the translation if something goes wrong, but if items are translated successfully, you can close the popup so that you don't have to watch items download one by one.

Furthermore, the items added for this identifier, perhaps should indicate that they might have pending downloads.

@dstillman a little explanation and question: currently translation happens, item(s) are created for the main (parent) items, then files are downloaded and if individual downloads succeed then their attachment items are created. Which means if you're on slow internet you'll first see parent item(s) without attachments and only when those files are downloaded attachments start showing up.

As @mvasilak hinted, would it be better to create all items immediately and then start all downloads (and show their progress the same way as if downloading them from our server)? This approach would be more complicated just because you'd have to handle interruptions and retry downloads (if users just close the app, downloads are interrupted and will have to be restarted after the app starts again). Also if people don't really download that many big papers at once, you most likely won't even notice the difference, because smaller papers are downloaded quickly and they just show up after few seconds. So I'm not sure whether it's worth it.

[dstillman]

Is the question about showing pending downloads in the lookup progress dialog or in the items list itself?

For the former, we know immediately from the translator whether there's an attachment, and we know the name to use. That's how we show the attachment line right away in the Zotero Connector save popup (and maybe the share extension?). So at least for that, we can show attachment progress without creating the attachment item first. Maybe we're already doing that?

If the question is about the items list, and we're talking about switching to a Close button once items are created but before downloads are done, could we hook into the lookup controller for the items list progress indicator, and also queue an 'open' action so that it opens the file once it exists? Seems very complicated, but I don't think we want to create an attachment item first, because that seems very likely to lead to attachment items with missing files, attachment items being synced before files are available, etc.

For a simpler option, could we just add an indicator/button somewhere that would reopen the lookup dialog to show progress? We do something similar for multi-item PDF metadata retrieval in the desktop app.

[michalrentka]

Is the question about showing pending downloads in the lookup progress dialog or in the items list itself?

The former is already implemented, it was the latter. I agree it's probably unnecessarily overcomplicated and we can create attachments only after they download and just ignore them if they don't.

For a simpler option, could we just add an indicator/button somewhere that would reopen the lookup dialog to show progress? We do something similar for multi-item PDF metadata retrieval in the desktop app.

That's a good idea. We could have a progress bar on bottom, same as when we're downloading multiple files (screenshot below) with a button to re-open the lookup popup. That way you'd know you're currently downloading and can view state as well.

It could say "Lookup x/y" with progress bar below and "Open" to the right.

[dstillman]

We could have a progress bar on bottom, same as when we're downloading multiple files (screenshot below) with a button to re-open the lookup popup. That way you'd know you're currently downloading and can view state as well.

👍

[dstillman]

"Lookup" is an internal term for the feature. Can it not just say "Downloaded x/y"?

[michalrentka]

"Lookup" is an internal term for the feature. Can it not just say "Downloaded x/y"?

Sure, I thought we might want to distinguish attachment downloads from lookup downloads. But I guess we can even combine them if both are in progress then.

[michalrentka]

@dstillman there's one more usability issue probably. When you add multiple identifiers, downloads are now in background, but translation happens sequentially. Translation could be moved to background too (by moving the underlying webview to parent controller). Do we want to do that? Or is it fine like this?

[dstillman]

What would the usability issue be? That translation stops if you close the app?

[michalrentka]

What would the usability issue be? That translation stops if you close the app?

Currently yes. The popup says "Cancel" while translations are in progress, so when you close the popup currently running translations are stopped. Started downloads finish in background.

[dstillman]

That seems fine.

[michalrentka]

That seems fine.

For me personally those translations took longer than file downloads, so I can't really close the window and see background downloads as we initially intended. This will only be visible for bigger file downloads. I guess that's not a problem. But if you add a lot of identifiers you'll have to wait for all of them to translate which can take some time. But it's still faster than adding them 1 by 1 through share extension.

[dstillman]

Ah, I see. So the question is if we want to change it to process them in the background with a Close button instead of (or in addition to) Cancel? And we added something to reopen the progress dialog, right? Does that button stick around after completion so you can view results?

[michalrentka]

Ah, I see. So the question is if we want to change it to process them in the background with a Close button instead of (or in addition to) Cancel? And we added something to reopen the progress dialog, right? Does that button stick around after completion so you can view results?

I guess we could add cancel to individual translations instead of general cancel and we'd keep Close for the whole window. The button to reopen dialog is not there yet, I'm just writing comment about that too.

[mvasilak]

The re-opening is still in development (PR is still WIP), but that is the goal. We can modify the items toolbar to say both
downloaded x/y and translated z/w.

[dstillman]

I guess we could add cancel to individual translations instead of general cancel and we'd keep Close for the whole window.

Why? Because some network request might be hanging? Maybe we want that, but seems weird not to also give the option to cancel the whole process if we have any cancel options.

translated z/w

"Saved z/w"

[michalrentka]

@mvasilak got it.

Some other issues:

• When single translation/download is in progress, the download progress in toolbar doesn't show up.
  https://github.com/zotero/zotero-ios/assets/1022956/41938fbd-dd2c-4518-b700-c0fe15b36fa9

• When I start translation on bad internet (I turned on network limiter), item and attachment are translated and I can see their titles, but then download progress is stuck at 0 and I don't see "downloading" in the toolbar. I see it only after it downloads a bit, do you know what it's waiting for?
  https://github.com/zotero/zotero-ios/assets/1022956/e8effc3b-35a2-4e83-b310-f63f65f95b98

[michalrentka]

I guess we could add cancel to individual translations instead of general cancel and we'd keep Close for the whole window.

Why? Because some network request might be hanging? Maybe we want that, but seems weird not to also give the option to cancel the whole process if we have any cancel options.

I thought cancel would make sense when the translation finishes and you see the resulted item is not what you wanted. But we can add cancel (all) and individual cancels or just cancel option.

[mvasilak]

@michalrentka I'd noticed that as well. Probably needs an initial event w/ 0 progress, similar to what AttachmentDownloader does. I will investigate it.

[dstillman]

I thought cancel would make sense when the translation finishes and you see the resulted item is not what you wanted.

The latter could be useful, but it should probably be an (x) delete button. And I guess that'd be fine for cancelling while in progress too.

[mvasilak]

New specs, from the discussion, please verify:

• Downloaded attachments from translated identifiers, are included in items toolbar label Downloaded x/y.
• New label Saved x/y will be added also in items toolbar, to show translation progress in the background.
• Button in items toolbar, will allow to re-open queue of translated identifiers.
• Lookup window will have Cancel all button, that will cancel all downloads and remove items/attachments already fetched. Probably it's good to show a confirmation alert.
• Lookup window will also have Close button, that just continues the process in the background.
• Each lookup item row will have a cancel button, for individual cancellation. Cancelling an item, will also remove its attachments. If item already downloaded, in a sense it becomes a delete/remove button, but could be the same, e.g. an X or trash icon.
• Each attachment row will also have a cancel button, just to cancel/remove this attachment.

A few things not commented already:

• Looking up new identifiers, could open the lookup input window, but when lookup is started, add them in the list with existing lookup items, if any.
• Lookup items should be removed at some point from the list. I guess when all their attachments have finished (successfully or otherwise), and if the lookup list window is not visible at the time.

[mvasilak]

@michalrentka regarding the sole download not appearing, it also happens for normal downloads of attachments, due to this guard

zotero-ios/Zotero/Scenes/Detail/Items/Models/ItemsState.swift

Line 35 in f9bfbf8

guard total > 1 else { return nil }

What is the reasoning behind this?
Should this edge case be maintained? If so I'll make this an option for the constructor.
Or could we make this check for total > 0 for all cases?

[michalrentka]

@michalrentka regarding the sole download not appearing, it also happens for normal downloads of attachments, due to this guard

zotero-ios/Zotero/Scenes/Detail/Items/Models/ItemsState.swift

Line 35 in f9bfbf8

guard total > 1 else { return nil }

What is the reasoning behind this? Should this edge case be maintained? If so I'll make this an option for the constructor. Or could we remove this check for all cases?

Yeah it made sense when it was used just for downloads. When you tap a single attachment which needs to download, you already see the download progress in the icon, so I ignored it. I guess just remove the guard.