-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #2899 - Scroll to Collections on change, animate new collections #3279
For #2899 - Scroll to Collections on change, animate new collections #3279
Conversation
9e18917
to
0697749
Compare
@@ -732,18 +739,56 @@ class HomeFragment : Fragment(), CoroutineScope, AccountObserver { | |||
emitAccountChanges() | |||
} | |||
|
|||
private fun scrollToCollections(addedTabsSize: Int) { | |||
launch(Dispatchers.Main) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Dispatchers.Main directly like this means there is no coroutineScope Job to handle cancellation. The fragment has a coroutineContext using Main already that will cancel and the delay() method will cooperatively cancel when it does so. Is there a reason not to use the current context and to create a new coroutineScope instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change work @colintheshots ?
0697749
to
138ca23
Compare
@@ -481,6 +484,10 @@ class HomeFragment : Fragment(), CoroutineScope, AccountObserver { | |||
} | |||
} | |||
} | |||
launch(Dispatchers.Main) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this dispatcher override here. Once this is done, I think we're ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops missed this one. Got it!
138ca23
to
df4ab2b
Compare
Pull Request checklist