Clean up StyleManager and respect dynamic type #65
Conversation
I saw you slack message. It seems like you were able to get CarPlay to work, were you able to check this PR? |
|
I did get the car play entitlement, but I haven’t had time yet to figure out how to launch the car play view controllers. I’ll figure that out next week. That’d be something good to add to the example app once that’s merged.
|
As I understand it, we support 1 or 2 styles, but never more than 2. Keeping styles in a potentially empty/boundless Array made the internals of this class more complicated than necessary. This is intended to be a no-op from a user facing perspective, though the API has changed and there may have been times before where `refreshAppearence` was called twice, where now we'll only call it once. Also changed: - Deduped some of "apply appropriate style" logic. - Better labeled the intent of tunnel style logic. Previously it took me a while to understand why were canceling the timer in `applyStyleType` (we dont want to unset .night theme if we happen to be in a tunnel at sunrise) Note: there remains a pre-existing bug: `preferredContentSizeChanged` will not re-apply until the style changes for some other reasons (like entering a tunnel or time of day change).
michaelkirk
force-pushed
the
mkirk/respect-dynamic-type
branch
from
2a67443 to
c26d727
Compare
| } | ||
| routeController.tunnelIntersectionManager(manager, willEnableAnimationAt: location) | ||
| // If we're in a tunnel at sunrise, don't let the timeOfDay timer clobber night mode | ||
| self.styleManager.cancelTimeOfDayTimer() |
There was a problem hiding this comment.
Good catch to cancel this when one is in a tunnel. But don't we need to re-add the timer once the user leaves the tunnel?
There was a problem hiding this comment.
Fair question! I should have called this out at the top level, but this is actually not a change in behavior. Previously this logic lived in applyStyle.
I was working on applyStyle and could not figure out why we'd cancel the timer. Eventually I realized it was only for this case, so I extracted the functionality here.
After you exit the tunnel, same as before, self.styleManager.timeOfDayChanged() is called which restarts the timer.
e.g. the green area roughly corresponds to a tunnel over the highway:
tunnel.mov.mp4
There was a problem hiding this comment.
A little more context in: f8eb874
Description
I broke this into two commits, it might be worth reviewing the commits separately. If you'd prefer, I can open them as two separate PRs.
The first commit changes the API of the StyleManger, and consequently cleans up its internals, but it's intended to be behaviorally a no-op.
Before:
After:
I'm unsure if it's a good idea to require the explicit
styleManager.ensureAppropriateStyle()after initialization. Previously this logic happend instyles.didSet.I could move it into the
StyleManger.init, but editing global state feels like a weird thing to do in an initializer.The second commit fixes a problem: changes to dynamic type were not being reflected until the style changed for some other reason. It looks like this was broken at some point (maybe in e134895)
Before: font size does not update
font-size-before.mov.mp4
After: font size updates immediately
font-size-fixed.mov.mp4
with small font
with big font
--
Note: I'm currently unable to test on CarPlay. Apparently you need some special entitlement from Apple even to run your app on the CarPlay simulator (is that right?!).
Even though I don't currently plan to use CarPlay in my own app, I've applied for the entitlement so I can test changes to like this that might brush up against it.