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

Added new result awaiting and setting ViewModel navigation #4848

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

entdark
Copy link
Contributor

@entdark entdark commented Apr 6, 2024

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Adds new IMvxResultAwaitingViewModel<TResult> and IMvxResultSettingViewModel<TResult> interfaces and corresponding abstract classes to create a result navigation contract between the awaiting and setting the result ViewModels. Under the hood it uses a new IMvxResultViewModelManager that registers, unregisters and sets the result to the registered result awaiting ViewModels.

It's better to use predefined abstract classes MvxResultAwaitingViewModel<TResult>, MvxMultiResultAwaitingViewModel<TResult1, TResult2>, MvxResultSettingViewModel<TResult>, MvxNavigationResultAwaitingViewModel<TResult>, MvxNavigationResultSettingViewModel<TResult>, and their overloads with parameters. Because when implementing IMvxResultAwaitingViewModel<TResult> and IMvxResultSettingViewModel<TResult> manually it requires implementing ViewModels registering, saving, restoring, unregistering, and setting the result manually as well.

⤵️ What is the current behavior?

No navigation with awaiting the result.

🆕 What is the new behavior (if this is a feature change)?

Navigation with awaiting the result that also survives Android restoration.

💥 Does this PR introduce a breaking change?

No.

🐛 Recommendations for testing

  1. Prepare an Android 13+ device or emulator since we need runtime Notification permission.
  2. Deploy the Playground.Droid app.
  3. Start it without debugging since the app will be shut down later.
  4. Click "SHOW CHILD" button - it will show a child Fragment on bottom
  5. Minimize the app (not close) by clicking Home or Overview.
  6. Go to the Playground.Droid app system settings.
  7. Make sure Notifications permission is granted.
  8. Disable Notifications permission.
  9. Go back to the app.
  10. The app will be restored with the child Fragment on bottom.
  11. Click "CLOSE" in the child Fragment.
  12. Check the logcat (via your preferred IDE): it should display
Info (26885) / RootViewModel: [Information] Result Playground.Core.Models.SampleModel from Playground.Core.ViewModels.ChildViewModel
  1. We received the awaited result after the full app restoration (with shutting down).

You can test this with any dangerous permissions.
Developer option "Don't keep activities" works differently - it does not shut down the app. Though the feature also works in this case.

For other platforms it just works without restoration issues.

📝 Links to relevant issues/docs

PRs: #4312 #4652
Issues: #3342 #3980 #4230 #4262 #4553 #4651

🤔 Checklist before submitting

🧠 Extra

I am open for discussing changes for the implementation.
Here are some ideas:

  • move 3 new navigation extensions to the navigation service directly
  • handle default implementations differently through default interface declaration
  • rename those long interface and class names to shorter ones

- survives Android restoration
- does not use Task to await the result
- uses callbacks to handle the result
- 3 new navigation extensions
- navigation with result put back into Playground
@Cheesebaron
Copy link
Member

Thanks will have a look at reviewing this one soon.

@picosmos
Copy link

picosmos commented Apr 12, 2024

@Cheesebaron Can you say when you think you'll be looking at this?

We are currently migrating a project and are considering whether to use the code from this PR or our own implementation from another project. (it's a similar strategy, but we would prefer to use framework features if possible. But unfortunately we can't wait that long anymore.)
Thank you for your great work :)

@entdark
Copy link
Contributor Author

entdark commented Apr 12, 2024

@picosmos
I think if you hurry then it's better to go with your own.
I am not pleased with the solution in a few places and also probably there will be review fixes.

@Cheesebaron
Copy link
Member

@Cheesebaron Can you say when you think you'll be looking at this?

We are currently migrating a project and are considering whether to use the code from this PR or our own implementation from another project. (it's a similar strategy, but we would prefer to use framework features if possible. But unfortunately we can't wait that long anymore.) Thank you for your great work :)

Sorry, I won't rush a review just because someone needs it. This looks something that would be easy to add to your own presenter override or use an alternative. Even if it gets reviewed and merged in, there is no guarantee there is a release with this in soon either.

I hope you understand :)

@picosmos
Copy link

@Cheesebaron Can you say when you think you'll be looking at this?
We are currently migrating a project and are considering whether to use the code from this PR or our own implementation from another project. (it's a similar strategy, but we would prefer to use framework features if possible. But unfortunately we can't wait that long anymore.) Thank you for your great work :)

Sorry, I won't rush a review just because someone needs it. This looks something that would be easy to add to your own presenter override or use an alternative. Even if it gets reviewed and merged in, there is no guarantee there is a release with this in soon either.

I hope you understand :)

Sure, that's the right way to do. I think we use our own implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/feature Feature request type
Development

Successfully merging this pull request may close these issues.

None yet

3 participants