refactor(ios/engine): Resource-download installation closures #3277
Conversation
| if let package = package { | ||
| do { | ||
| // A raw port of the queue's old installation method for lexical models. | ||
| try ResourceFileManager.shared.finalizePackageInstall(package, isCustom: false) |
There was a problem hiding this comment.
Note - at this stage, we could very easily just install the language pairing that was requested.
That said, I believe we've stated an intention to install lexical models for all supported languages, rather than just the one. Either way, the plan is to (eventually) replace the finalizePackageInstall call with something more explicit.
Currently kept in this form for ease of comparison to lines 525-535 of ResourceDownloadQueue.
| batch.completionBlock?(package, nil) | ||
| } | ||
| let packagePath = URL(fileURLWithPath: task.request.destinationFile!) | ||
| batch.completeWithPackage(fromKMP: packagePath) |
There was a problem hiding this comment.
So, so smooth how this resolves within ResourceDownloadQueue now. Once we download KMPs for keyboards, this function's if-else ladder can be dropped entirely, greatly simplifying this part of the code.
| ResourceDownloadManager.shared.downloadKeyboard(withID: withID, | ||
| languageID: languageID, | ||
| isUpdate: isUpdate, | ||
| fetchRepositoryIfNeeded: fetchRepositoryIfNeeded) | ||
| fetchRepositoryIfNeeded: fetchRepositoryIfNeeded, | ||
| completionBlock: completionBlock) |
There was a problem hiding this comment.
Examining this function in retrospect, I'm not sure if functioned properly before. The code that would ensure that the downloaded keyboard was added to the user's installed keyboard list wasn't included then.
|
|
||
| if Manager.shared.currentKeyboard?.fullID == keyboard.fullID { | ||
| // Issue: does not actually trigger a reload if the user isn't within the Settings view hierarchy | ||
| // Fixing this requires a refactor of `shouldReloadKeyboard`. |
There was a problem hiding this comment.
This should be done within its own PR. I already know what needs to be done and the approximate scale involved, but I believe it should be triaged until later in the sprint; we have more urgent things to address.
(shouldReloadKeyboard should be on the InputViewController and read whenever the input view is reloaded, rather than the current Manager-based hackery.)
jahorton
deleted the
refactor/ios/engine/download-installation-completion-blocks
branch
This extracts the data-management aspects of resource installation from the various KeymanEngine views and defines it in the same class as the actual installation methods. I've set things up with a few functions that configure installation closures that may be set to run when downloads complete. This helps to ensure installation consistency regardless of origin in the view hierarchy while also allowing further configurability / customization as needed.
This doesn't add unit tests for all the recent downloader-refactoring work; the plan is to address that once KeymanEngine is downloading keyboard KMPs. At that point,
ResourceDownloadQueuewill be far more stable. While it's already more test-friendly than before, use of keyboard KMPs will improve things on that level as well.