Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR]: Misused of DisposableEffect instead of LaunchedEffect #515

Open
2 tasks done
mhdabbaghy opened this issue Dec 29, 2022 · 6 comments
Open
2 tasks done

[FR]: Misused of DisposableEffect instead of LaunchedEffect #515

mhdabbaghy opened this issue Dec 29, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@mhdabbaghy
Copy link

mhdabbaghy commented Dec 29, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the problem

in MainActivity to update the dark content of the system bars to match the theme, there is a DisposableEffect which onDispose method is empty with no comment

DisposableEffect(systemUiController, darkTheme) {
    systemUiController.systemBarsDarkContentEnabled = !darkTheme
    onDispose {}
}

this code exists in MainActivity:103

Describe the solution

I can update it and use LaunchedEffect instead

Additional context

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mhdabbaghy mhdabbaghy added the enhancement New feature or request label Dec 29, 2022
@hoc081098
Copy link
Contributor

hoc081098 commented Dec 29, 2022

I think DisposableEffect is more suitable. LaunchedEffect is used to call suspend functions other than normal functions

Sent from my 2201117TG using FastHub

@mhdabbaghy
Copy link
Author

mhdabbaghy commented Dec 29, 2022

@hoc081098 I do agree with you about not using suspend function but we are not disposing anything at all

@hoc081098
Copy link
Contributor

I think, an empty onDispose{} is not bad

@SimonMarquis
Copy link
Contributor

SimonMarquis commented Aug 24, 2023

@mhdabbaghy I also think this is the right usage of DisposableEffect based on it's javadoc.
The onDispose method is empty because there is not special code associated to this callback, but it has to be returned from the DisposableEffect call.
This issue should be closed, unless you have additionnal feedback :)

@dev-weiqi
Copy link

@SimonMarquis It's a bit of a conflict with the official docs 👀

截圖 2023-10-08 00 05 34

@alexvanyo
Copy link
Contributor

Agreed that this is not an ideal use of DisposableEffect, as it leads to an empty onDispose which is generally to be avoided.

The reason DisposableEffect is used over LaunchedEffect in this particular case is to update the system UI for a new dark mode setting as soon as possible: If the code was in a LaunchedEffect, it would run slightly later due to dispatching than when it runs now when it is placed in a DisposableEffect, and window flags are especially sensitive to timing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants