Conversation
theospears
left a comment
theospears
left a comment
There was a problem hiding this comment.
I agree this is needed! Added a few questions, but also happy to merge as-is if you think this is the best approach.
BeeKit/Managers/RequestManager.swift
Outdated
| case .success(let data): | ||
| let asJSON = try? JSONSerialization.jsonObject(with: data) | ||
| return asJSON | ||
| case .success(let data): return try await Task.detached { try JSONSerialization.jsonObject(with: data) }.value |
There was a problem hiding this comment.
What are your thoughts on this solution vs making RequestManager an actor so all its work is done on a background thread?
There was a problem hiding this comment.
Switching it over to actor seems like a more complete solution.
There was a problem hiding this comment.
What are your thoughts on this solution vs making RequestManager an actor so all its work is done on a background thread?
I think the switch to RequestManager makes sense, and its public API is marked as asynchronous anyway already. Although it currently does not maintain much internal state, it does interact with AF.request.
Also, marking it as an actor I think does not make its work happen off of the main thread. Thus the CPU-intensive work of JSON parsing is moved onto a background thread, explicitly.
There was a problem hiding this comment.
What are your thoughts on this solution vs making RequestManager an actor so all its work is done on a background thread?
bumped RequestManager to an actor and introduced the protocols for signed and non-signed requests, which have been used on the testing side as well.
There was a problem hiding this comment.
Now it is an actor do we still need this Task.detached?
There was a problem hiding this comment.
I believe so: we still want the work to not block.
krugerk
force-pushed
the
bugfix/laggy-gallery
branch
4 times, most recently
from
53fefad to
d582c89
Compare
krugerk
force-pushed
the
bugfix/laggy-gallery
branch
2 times, most recently
from
e3f4fc2 to
d844e4a
Compare
| func addDatapoint(urtext: String, slug: String, requestId: String?) async throws -> Any? | ||
| } | ||
|
|
||
| extension RequestManaging { |
There was a problem hiding this comment.
Can you explain what this is for? It seems like functions just calling themselves
There was a problem hiding this comment.
The protocol cannot have a default for the parameter and several places were calling them without specifying the parameter with a nil.
This here is a version that does (pass nil as a default) and allows then others to make calls without specifying also the parameters: nil for each and every call.
by serializing json off of the main thread
by prefetching gallery's thumbnails
krugerk
force-pushed
the
bugfix/laggy-gallery
branch
from
d844e4a to
e3e8cf6
Compare
2 participants
Summary
Upon returning to the gallery, especially after bring the app back into the foreground, scrolling the gallery can be very choppy.
This PR reduces some of the lagginess by moving json serializing off of the main thread and by prefetching the gallery's thumbnails.
RequestManager was made an actor and it was made to implement protocols to make testing/mocking easier.
Validation
Used app in Simulator
logged out,
logged in,
browsed goal list (gallery),
opened goals (goal view),
edited and deleted datapoints from without the main app,
loaded settings
#588 mentioned a slow gallery.