Skip to content

Conversation

@TimoPtr
Copy link
Member

@TimoPtr TimoPtr commented Dec 1, 2025

Summary

This PR cleans up the OnboardApp class and keep it only for the wear onboarding since it is not needed for the rest while removing all the useless parameters.

Checklist

  • New or updated tests have been added to cover the changes following the testing guidelines.
  • The code follows the project's code style and best_practices.
  • The changes have been thoroughly tested, and edge cases have been considered.
  • Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Any other notes

I tested the onboarding on TV, mobile and wear.

Comment on lines +178 to +179
// Active the newly added server
serverManager.activateServer(serverId)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is for Wear, and it's also a behavior change I don't fully understand the reasoning behind. Can you elaborate / pull it out into another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was called in the results of the onboarding after adding a server. If you look at the old code activity result in settings you would see that. I moved it here so it replicates the same behavior as before.

Copy link
Member

Choose a reason for hiding this comment

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

For app usage it may replicate the same usage, but now it also tries to active the Wear server on the phone that started onboarding it, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably not clear enough and would deserve a comment in the nameyourweardevice package that actually it uses the screen but not the view model actually.

So no it doesn't activate the server on the wear onboarding.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please add a comment as that seems like something you may easily miss like I did

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants