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

Refactor: Use shared StartStopTypedEventHandler for Sensors #19166

Conversation

nickodei
Copy link
Contributor

@nickodei nickodei commented Jan 7, 2025

GitHub Issue: closes #19131

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

What is the current behavior?

Most sensors used an TypedEventHandler field and had duplicated logic on the event handling.

What is the new behavior?

Refactoring to use StartStopTypedEventHandler that encapsulates the logic for event-handling.
This is now used in the sensor classes instead.

PR Checklist

Please check if your PR fulfills the following requirements:

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform labels Jan 7, 2025
@nickodei
Copy link
Contributor Author

nickodei commented Jan 7, 2025

I also saw that on a lot of sensor classes, the GetDefault() Method was duplicated as well. In LightSensor we use Lazy<LightSensor?> _instance. Should this also be done for the other sensors? I wasn't sure about it.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19166/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 151254 has failed on Uno.UI - CI.

@github-actions github-actions bot added the platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform label Jan 10, 2025
@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19166/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19166/index.html

Copy link
Member

@morning4coffe-dev morning4coffe-dev left a comment

Choose a reason for hiding this comment

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

Thank you for your PR, @nickodei! I tested the changes with Android and it seems to be working well. The changes look good to me, any suggestions @MartinZikmund?

@MartinZikmund
Copy link
Member

Looks good to me! Merging!

@MartinZikmund MartinZikmund merged commit 4bc6ea1 into unoplatform:master Jan 14, 2025
110 checks passed
Copy link

welcome bot commented Jan 14, 2025

Congrats on your first pull-request! We ❤ the people who are involved in this project, and we’d love to pay it forward by sending you some swag. If you have any feedback (or ideas how to improve Uno as a open-source project) please open a feedback issue.
giphy
Merci beaucoup 👋

@MartinZikmund
Copy link
Member

I also saw that on a lot of sensor classes, the GetDefault() Method was duplicated as well. In LightSensor we use Lazy<LightSensor?> _instance. Should this also be done for the other sensors? I wasn't sure about it.

That can be done as separate PR - I guess using Lazy for all sensors might be the best way to unify them all (and it will clean up all the _initializationAttempted variables too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform platform/ios 🍎 Categorizes an issue or PR as relevant to the iOS platform platform/wasm 🌐 Categorizes an issue or PR as relevant to the WebAssembly platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use StartStopEventWrapper for sensor events
5 participants