Add Font Provider Before Backend#632
Conversation
anli5005
left a comment
anli5005
left a comment
There was a problem hiding this comment.
I'd personally address the force unwrapping before we merge, but other than that it LGTM. If you wanted to, you could feature flag this as ad-hoc and merge it in before backend is ready
| // Note, if we get to this point, unit.lottie really should not be nil, neither should the fontProvider | ||
| let progressFraction = (CGFloat(vm.activeUnitProgress) * unit.time!) / unit.lottie!.duration | ||
| let normalizedProgress = progressFraction - floor(progressFraction) | ||
| GeometryReader { proxy in | ||
| LottieView(animation: unit.lottie!) | ||
| .textProvider(DictionaryTextProvider(unit.values)) | ||
| // TODO: Define a custom font provider conforming class that fetches fonts dynamically from backend. | ||
| .fontProvider(DefaultFontProvider()) | ||
| .fontProvider(experienceVM.model.fontProvider!) |
There was a problem hiding this comment.
I'm not a fan of trusting that neither is nil here. Can we engineer this in a more type-safe way to eliminate the possibility of crashes like the one seen in #630?
There was a problem hiding this comment.
It's the "trust me bro" guarantee, but yes I (or someone on iOS) can look into.
The thought here is that WrappedUnitView shouldn't even be showing for a given model if that model doesn't have a lottie animation loaded, same thing with the font provider. But there does exist a mechanism to guarantee, so you're correct in that regard.
I don't want it on prod devices until at least /current is handled, because it will just cause a spike in sentry errors. |
| @@ -47,5 +64,49 @@ public struct WrappedModel: Decodable { | |||
| } | |||
|
|
|||
| self.pages = newPages.filter({ $0.lottie != nil }).sorted(by: { $0.id < $1.id }) | |||
There was a problem hiding this comment.
@anli5005 This isn't what you mean but WrappedUnitView cannot display before we have a loaded model
| } | ||
| } | ||
|
|
||
| DispatchQueue.main.async { |
There was a problem hiding this comment.
await MainActor.run
Or just mark the entire Task as @MainActor
2 participants
NOTE: THIS SHOULD NOT BE MERGED BEFORE BACKEND FINISHES THEIR IMPLEMENTATION AND IT HAS BEEN ADEQUATELY TESTED (but it should work)
Dynamically rendered text requires the original font used for the design (instead of non-dynamic text, which relies on vector graphics). For 2025S, we just hardcoded the fonts but alluded to an AnimationFontProvider-conforming class, which is this PR.
Backend should provide a dictionary of
[String: URL]whose keys are individual font names, and whose URLs are links to ttf font files. This way, we download all the necessary fonts for wrapped in the loading phase, then allow the animation to reference these fonts. Again, this hasn't been tested, since there's no backend infra for this. However, the changes required as a result of their implementation will be minimal.Other things:
api/wrapped/currentinstead of a hard-coded semester. This is the real reason that this PR should wait, since they currently don't error handle unknown semester names, so it was causing 2.5k sentry errors. But this is in the works (where they can identify a certain semester to be pointed to from /current)