Skip to content

Conversation

@Jaehwa-Noh
Copy link
Contributor

@Jaehwa-Noh Jaehwa-Noh commented Mar 2, 2024

What I have done and why
I've added withContext(defaultDispatcher) for main-safe at combine.

Fix #1420

DefaultSearchContentsRepository.searchContents

Before After
image image

DefaultSearchContentsRepository.getSearchContentsCount

Before After
image image

GetFollowableTopicsUseCase.invoke

Before After
image image

Flow.mapToUserSearchResult

Before After
image image

Copy link
Contributor

@hoc081098 hoc081098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

combine(a, b) { ... }
    .flowOn(defaultDispatcher)

I think the above code has the same meaning.

image

@Jaehwa-Noh
Copy link
Contributor Author

combine(a, b) { ... }
    .flowOn(defaultDispatcher)

Good suggestion. I'll accept your suggestion.
Thank you.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft March 4, 2024 09:53
@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review March 4, 2024 12:13
@dturner
Copy link
Collaborator

dturner commented Mar 5, 2024

Getting a build error:

e: file:///home/runner/work/nowinandroid/nowinandroid/core/data/src/test/kotlin/com/google/samples/apps/nowinandroid/core/data/CompositeUserNewsResourceRepositoryTest.kt:40:9 No value passed for parameter 'defaultDispatcher'

@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft March 6, 2024 06:37
@Jaehwa-Noh Jaehwa-Noh changed the title Apply withContext(defaultDispatchers) for main-safe. Apply flowOn(defaultDispatchers) for main-safe. Mar 8, 2024
@Jaehwa-Noh
Copy link
Contributor Author

#1250 This issue had given the clue that how to solve the test fail problems when I was struggling with some tests fail.
The key was single scheduler!

Thanks.

@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review March 8, 2024 07:00
@Jaehwa-Noh Jaehwa-Noh marked this pull request as draft May 9, 2024 09:45
@Jaehwa-Noh Jaehwa-Noh marked this pull request as ready for review May 9, 2024 11:18
@keyboardsurfer
Copy link
Member

Please resolve the remaining merge issue so we can proceed to merge this PR.

@Jaehwa-Noh
Copy link
Contributor Author

@keyboardsurfer I resolved a conflict and applied a spotless.

@keyboardsurfer
Copy link
Member

The error you're seeing is from a dependency issue which has been resolved on main. Please rebase your PR.

Change-Id: I04d005004d4fd6b813e625a1865edf5b65f2a5c8
Change-Id: I81c5e020fe2632f1c1ad6ca411df59fb0e867ce9
@Jaehwa-Noh
Copy link
Contributor Author

@keyboardsurfer Thank you for waiting me solving a conflict. I rebased it, and it's ready to merge.

Change-Id: Id4ec69492518663bdbae5c4965183f7b6dff8cb9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Some combine works on Main thread

4 participants