Add Android Preload List (DNM) #260
Conversation
| val viewModel = ViewModelProvider( | ||
| this, | ||
| SitePreparationViewModelFactory(application, configurationItem) | ||
| )[SitePreparationViewModel::class.java] |
There was a problem hiding this comment.
⛏️Aren't we using injection in the example project? ViewModels can be simply injected.
| * @param filePath The absolute path to the serialized dependencies file. | ||
| * @return The deserialized dependencies, or `null` if the file doesn't exist or is invalid. | ||
| */ | ||
| fun readFromDisk(filePath: String?): EditorDependencies? { |
There was a problem hiding this comment.
⛏️ kotlin nitpick: Do we need to accept null for filePath? I would avoid nullability when possible.
| null | ||
| } | ||
|
|
||
| val stringValue = jsonValue?.toString() ?: "" |
There was a problem hiding this comment.
| val stringValue = jsonValue?.toString() ?: "" | |
| val stringValue = jsonValue?.toString().orEmpty() |
| /** | ||
| * Shuts down background operations. | ||
| */ | ||
| fun shutdown() { |
There was a problem hiding this comment.
I don't see this function being called. So, creating a CoroutineScope as a variable could cause leaks because of no lifecycle awareness..
❓ What about using the ´ApplicationScope´, ´ViewModelScope´, or similar?
| responseHeaders = EditorHTTPHeaders(entry.headers) | ||
| ) | ||
| } catch (e: Exception) { | ||
| null |
There was a problem hiding this comment.
Do we want to track the exception here?
|
|
||
| override fun onDetachedFromWindow() { | ||
| super.onDetachedFromWindow() | ||
| coroutineScope.cancel() |
There was a problem hiding this comment.
coroutineScope is dead forever.
There are a few lifecycle aware CoroutineScopes we can use on Android. This class is a tricky one, but maybe you could try using:
findViewTreeLifecycleOwner()?.lifecycleScope.run {
...
}
dcalhoun
force-pushed
the
add/preload-list-tests
branch
from
209696f to
f59acb1
Compare
jkmassel
force-pushed
the
add/preload-list-tests
branch
from
f59acb1 to
c82027d
Compare
jkmassel
force-pushed
the
add/android-preload-list
branch
2 times, most recently
from
a8fb889 to
d33bb53
Compare
jkmassel
force-pushed
the
add/android-preload-list
branch
from
d33bb53 to
0e760f1
Compare
jkmassel
force-pushed
the
add/android-preload-list
branch
from
0e760f1 to
8060669
Compare
Full warning: > Redundant creation of Json format. Creating instances for each usage can be slow.
What?
Why?
How?
Testing Instructions
Accessibility Testing Instructions
Screenshots or screencast