I am going break this into three parts:
- Your question about the observer.
- Some big picture questions about the broader design.
- And a ton of tactical observations regarding the code in the question.
Regarding the observation, yes, it is perfectly reasonably to have the view model publish updates to this array of objects.
What feels a little strange, though, is for the DownloadViewControllerRepresentable to be observing it rather than the DownloadViewController itself. It is not necessarily wrong, but just feels odd.
Personally, I would want to make the view controller to be a highly-cohesive, loosely-coupled object, availing itself of its own view model to monitor progress. Sure, abstract the business logic about how to fetch updates into the view model, but it feels deeply wrong to entangle the UIViewControllerRepresentable for this. The UIViewControllerRepresentable is just a shim/bridge, not a functional object (IMHO).
The view controller should be a self-contained “view”, with all the logic necessary to produce the desired UI. The UIViewControllerRepresentable is a bridge from SwiftUI to UIKit. I might suggest that it is the view controller that should have the view model logic, not this shim between SwiftUI and UIKit.
Another way of thinking about it: You should be able to drop this DownloadViewController into a UIKit project, and have it work there, too. But the way this is written, the UIViewControllerRepresentable owns the view model, which is required to have the view controller function properly.
A few big picture observations:
Why are you using UIKit at all?
A table with cells with names and progress views is well within the capabilities of SwiftUI, and I am not sure why you would go through all of this. It feels horribly and unnecessarily overengineered.
We tend to avail ourselves of these UIViewRepresentable and UIViewControllerRepresentable types in one of these two following scenarios:
- We are refactoring some legacy codebase and just have not yet gotten around to refactoring this particular view; or
- We need to present some UI that is not currently easily achieved in SwiftUI.
But, perhaps I am overlooking something. Why precisely are you using “representable” UIKit views at all?
Why are you polling every three seconds?
If the API really does not provide any observation of progress or similar callbacks, then fine, you have to do what you have to do. But, anytime we see polling like this in an application, it feels deeply inefficient and wrong.
We now live in a event-driven world, and polling is an anachronism. Again, hey, if that is really the only mechanism your API supports, then that is fine. But, it really is a shockingly primitive way of doing it.
Do you really want to reload cells in your table view when progress changes?
The diffable data source pattern really excels at inserting rows, removing rows, or replacing rows. It is a brilliant system. But in this case, you probably have a bunch of rows whose progress is getting updated, and you really do not want to be reloading the whole cell. You want to animate the slider on a progress view, not reloading the whole cell.
You have not shared the details on the API you are using, so we cannot comment further, but entirely reloading cells (despite the elegance of diffable cells) when you really only want to animate progress views within those cells feels wrong.
Now, doing this properly may well beyond the scope of what you can contemplate right now, but ultimately, you want to differentiate between diffable data sources (where cells are reloaded) and progress updates for an existing cell.
Where is the “source of truth”?
Often, when thinking about the data flow within SwiftUI apps (or really, any apps), it is useful to think about what is the “source of truth”. See WWDC 2019’s Data Flow Through SwiftUI.
I ask because if the presenting view initiated the downloads, then perhaps its models should be the source of truth, and it might just provide bindings to this view.
The way this question was presented, it is hard to say whether it is correct for this view controller to have its own model interacting with this API or whether it should avail itself of bindings to some other model that is getting updated already.
We cannot provide a definitive answer here, as there is not enough information about this API, about what the presenting view is doing etc. But it is something to factor into your thought process.
A bunch minor observations on the code snippet provided in the question:
I would avoid using Data for your model object name. It is just confusing to use the name of a framework type for your model object. Now, if this is a name that the API is forcing upon you, so be it; but, if so, shame on them.
I would avoid the label name of myLabel. I might suggest sourceNameLabel.
I would avoid littering the view controller with string literals. E.g., I might make the nibName and reuseIdentifier static constants in the cell definition. Or, perhaps better, hide these details from the view controller entirely
class DownloadViewCell: UITableViewCell {
private static let reuseIdentifier = "DownloadViewCell"
private static let nibName = "DownloadViewCell"
@IBOutlet weak var sourceNameLabel: UILabel!
@IBOutlet weak var progressView: UIProgressView!
func updateProgressView(_ progress: Float) {
progressView.progress = progress
}
}
// MARK: - Table view helpers
extension DownloadViewCell {
static func register(for tableView: UITableView) {
let nib = UINib(nibName: DownloadViewCell.nibName, bundle: nil)
tableView.register(nib, forCellReuseIdentifier: reuseIdentifier)
}
static func dequeue(for tableView: UITableView, indexPath: IndexPath) -> DownloadViewCell {
tableView.dequeueReusableCell(
withIdentifier: Self.reuseIdentifier,
for: indexPath
) as! DownloadViewCell
}
}
Then the view controller can do:
func configureTableView() {
DownloadViewCell.register(for: tableView)
dataSource = UITableViewDiffableDataSource<Int, Download>(tableView: tableView) { tableView, indexPath, dataItem in
let cell = DownloadViewCell.dequeue(for: tableView, indexPath: indexPath)
cell.sourceNameLabel.text = dataItem.sourceName
cell.updateProgressView(dataItem.progress / 100)
return cell
}
tableView.dataSource = dataSource
}
You have created a diffable data source, but:
- You never set the table view’s data source to be this diffable data source;
- You have included the implementation of legacy
UITableViewDataSource methods, which can be retired entirely now that you have a diffable data source.
Your timer has introduced a strong reference cycle. The attempt to clean this up in deinit will not work, because the timer is keeping a reference to self. Use [weak self] capture list.
The if let can be simplified, replacing:
if let queryList = queryList {…}
With:
if let queryList {…}
The timer fires every three seconds. I would suggest that you do not want to do that (what if a request took over three seconds; you would now have overlapping requests). You really want to initiate the next request some fixed amount of time after the prior one finishes (not after you started the prior one).
To that end, fetchDownloads does not provide any way of knowing when it finished. Whenever you have a method that initiates an asynchronous task, it should provide a mechanism to know when the asynchronous work is done. If calling a completion-handler-based method, give your method a completion handler, too.
Or better, I might adopt async-await:
@MainActor
class DownloadListViewModel: ObservableObject {
@Published var downloadList: [Data] = []
let qnapApi = ApiClientQnap.shared
var task: Task<Void, Never>!
init() {
task = Task { [weak self] in
while !Task.isCancelled {
await self?.fetchDownloads()
try? await Task.sleep(for: .seconds(3))
}
}
}
deinit {
task.cancel()
}
func fetchDownloads() async {
if let response = await qnapApi.performQuery() {
downloadList = response.data ?? []
}
}
}
And if this ApiClientQnap does not provide an async rendition, you can write your own:
extension ApiClientQnap {
func performQuery() async -> QnapResponse? {
await withCheckedContinuation { continuation in
performQuery { response in
continuation.resume(returning: response)
}
}
}
}
Now, I do not know what type the completion handler of performQuery provided, so I just called it QnapResponse, but use whatever type that API actually uses.
Your are using snakecase property names (e.g., source_name), rather than camelcase (i.e., sourceName). The latter is the established convention in Swift. I would shift to camelcase. If you are doing this because you are decoding JSON that used snakecase keys, the keyDecodingStrategy of convertFromSnakeCase will do this conversion for you automatically. Again, it is hard to comment further without understanding why you introduced snakecase property names in the first place.
