GT-2491 resume language download progress

[rachaelblue]

Hey @levieggertcru, I've gone around and around with this and it's generally working with one hiccup:

If you exit in the middle of a download and then immediately reopen the page, the progress wheel shows correctly where it left off, but frequently it then jumps back to 0 and starts the progress wheel over. From what I've seen in testing, I think this could be something about the way requests get sent and counted. Granted, it doesn't happen 100% of the time, sometimes it just finishes and that's it.

I did try making a test project to see if I could attach a new .sink to an old publisher reference, but I was struggling to create similar enough conditions (like using SwiftUI navigation verses UIKit makes a difference lifecycle wise.) So I ended up just sticking an extra publisher into LanguageDownloadable that listened to userDefault changes and reattaching to that publisher when the page closed/opened, and that worked fine. Which made me think that adding a new sink onto the old download publisher probably wasn't the cause of seeing "double requests". Then when I went your route of having the static class just hold onto the sink, it seems suspicious that there's still "double progress", but I just can't quite nail down why that's happening. 🤔

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.94%. Comparing base (d9d451f) to head (c3289c2).
Report is 136 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2444      +/-   ##
===========================================
+ Coverage    95.92%   95.94%   +0.01%     
===========================================
  Files           62       62              
  Lines         5323     5323              
===========================================
+ Hits          5106     5107       +1     
+ Misses         217      216       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[levieggertcru]

Trying to remember where I saw this, but you can also access class level variables using keyword Self capitalized "s".

[rachaelblue]

Oh cool!

[levieggertcru]

Hey @rachaelblue that's really odd! I will need to do some debugging into this to see.

I'm wondering is there still an issue with the double progress?

And it sounds like in your test project you were having success when updating UserDefaults?

[levieggertcru]

Hey @rachaelblue do you know if this is keeping a strong reference since using assign on a static reference?
Does the ViewModel get deallocated? I'll try running this PR soon to see.
Also my original thought was that the progress would get updated through the delegate handler. Was that causing issues though with Double progress?

[levieggertcru]

Just checked by tapping a few languages to download and when I navigated back I did see the deinit log for the DownloadableLanguagesViewModel so that's good to know!

[levieggertcru]

Hey @rachaelblue I was wondering too if some of the attributes in DownloadableLanguageItemView should be marked State

godtools-swift/godtools/App/Features/AppLanguage/Presentation/DownloadableLanguages/Subviews/DownloadableLanguageItemView.swift

Line 20 in c3289c2

@State private var progressAnimationTimer: Timer?

Do these need to be marked with State? Especially the Timer's? I don't think those need to be, does the view update anything when those values change?

State private var downloadProgressTarget: Double
State private var progressAnimationTimer: Timer
State private var removeDownloadTimer: Timer
State private var isVisible: Bool

[levieggertcru]

Hey @rachaelblue I'm gonna have to take a deeper look to see what's happening in the language downloading. I created a very simple test project that uses a Timer to simulate the download progress. When I deallocate the ViewModel and come back in I resubscribe to existing publishers static reference and the download picks right up where it last left off. So, something wonky is happening in there.

[levieggertcru]

Hey @rachaelblue I see why the Timers are marked with the State property wrapper. Mainly because it's being mutated and also captures self within the block when the timer fires. It looks like using State is a way to get around mutating the View (value type).

I do wonder if this is a good practice though since State isn't being utilized as intended (updating the UI when State changes). I'm going to do a little more research into best practices for using Timers in SwiftUI Views.

The Timers are getting invalidated and set to nil on View disappear which is good, otherwise there would be a reference cycle.

[levieggertcru]

Hey @rachaelblue I checked on the Timer and I remember you were looking into this before. Timer offers a Timer.TimerPublisher and methods connect() and autoconnect() to start the timer and method cancel() to stop the timer. Only issue is, if you stop the timer, it can't be started again unless a new instance is created. So we'd need use State on the Timer.TimerPublisher.

[rachaelblue]

Hey @levieggertcru, yeah I remember State allows you to mutate things on the struct. I didn't dig into any of the view code this time around, so I'm kinda fuzzy on how that part works. Was mainly testing out the publishers. But it certainly could be related to the timer that things are weird. The whole setup is tricky.

[levieggertcru]

Hey @rachaelblue I'm seeing now that there are a lot of moving parts and some definite nuances to think about. Even scrolling items on and off screen has some nuances.