Skip to content

Fix activation for nested conductor / screen scenarios #898

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

Merged
merged 12 commits into from
Nov 11, 2024

Conversation

Yinimi
Copy link
Contributor

@Yinimi Yinimi commented Apr 2, 2024

There have been some discussions and changes related to when OnActivate() is called.
#593 He wanted to use IsInitialized / IsActive to control the visibility of some loading indicator. If the property is set to true before the OnActivate / OnInitialize call it will obviously not be able to use it for determining if the loading has finished.
I can understand the reason for this request but I think a method called OnActivate() should be called after the IsActive is set to true, otherwise Activating or BeforeActivate are better names in my opinion.

Since some devs (like me) want to use OnActivate() to trigger the initializing of the ViewModel there are scenarios where it is problematic if the IsActive is not set yet. I created the ActivateWhileActivateOneLevel() test method to demonstrate it.
To provide such a method @erik-hooper added with #859 a OnActivated() method which is called after the property has been set.
If I use this method with another test ActivatedWhileActivatedOneLevel() The issue is resolved.

I have a more complicated and nested scenario. example Test

  • Outer Conductor
    • some Page
    • Inner Conductor
      • View 1
      • View 2

In my app the inner conductor handles the navigation depending on some configuration.
The outer conductor handles basic stuff and also the display of the user preferences ("some Page").
If the View 1 now wants automatically to continue after the user preferences have been shown (think of auto login was just enabled) then in this nested scenario the View 2 will not be initialized because all this is triggered by the OnActivate() method of the inner conductor.

With this change the behavior should be like before the changes of #593 where the active item(s) are activated after the conductor has IsActive enabled.

In #789 it was also discussed to add something like BeforeActivate and AfterActivate.
I added a OnInitializedAsync() to adress this as well. I didn't touch the OnViewLoaded (it was wished to make this async), this looks more complicated.

@Yinimi
Copy link
Contributor Author

Yinimi commented Apr 2, 2024

If the next release is 5 then it would also be ok to rename the methods. (OnActivate and OnInitialize)
Eventually OnActivate => BeforeActivate, OnActivated => OnActivate (to make things as complicated as possible)

@vb2ae vb2ae self-assigned this Apr 7, 2024
@vb2ae
Copy link
Member

vb2ae commented Apr 7, 2024

Yes the next release is version 5

@Yinimi
Copy link
Contributor Author

Yinimi commented Apr 8, 2024

I added the obsolete messages

@Yinimi
Copy link
Contributor Author

Yinimi commented Apr 9, 2024

I think #764 Is also resolved with this change

@a44281071
Copy link

asp.net Blazor use void OnAfterRender() and Task OnAfterRenderAsync() for view update. And use void OnInitialized() and Task OnInitializedAsync() for init load.

ASP.NET Core Razor component lifecycle:

Component initialization

@Yinimi
Copy link
Contributor Author

Yinimi commented Apr 19, 2024

If I read the build pipeline log correct the android 28 sdk is missing? Is the pipeline running on a specific machine with a build agent? Then it would probably be required to install it there manually

@vb2ae
Copy link
Member

vb2ae commented Apr 21, 2024

Working on a fix for android version in another branch

Copy link
Member

@vb2ae vb2ae left a comment

Choose a reason for hiding this comment

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

Looks good. Could you please update the Feature apps with the change

@vb2ae vb2ae added this to the Version 5.0 milestone Nov 5, 2024
@Yinimi Yinimi closed this Nov 6, 2024
@Yinimi Yinimi reopened this Nov 8, 2024
@Yinimi
Copy link
Contributor Author

Yinimi commented Nov 8, 2024

@vb2ae I updated the Feature branch.
Additionally I fixed the scenarios solution but it looked very outdated, it was still using some caliburn micro version 3 dlls.

@vb2ae
Copy link
Member

vb2ae commented Nov 8, 2024

@Yinimi thank you the samples do need help.

@Yinimi
Copy link
Contributor Author

Yinimi commented Nov 8, 2024

There seems to be a related issue #906
The changes cause unit tests to fail

@vb2ae
Copy link
Member

vb2ae commented Nov 8, 2024

@Yinimi I will look at it

@vb2ae vb2ae mentioned this pull request Nov 11, 2024
Copy link
Member

@vb2ae vb2ae 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 finding this

@@ -39,12 +39,13 @@ object IHaveActiveItem.ActiveItem
/// <returns>A task that represents the asynchronous operation.</returns>
protected virtual async Task ChangeActiveItemAsync(T newItem, bool closePrevious, CancellationToken cancellationToken)
{
var previousItem = _activeItem;
Copy link
Member

Choose a reason for hiding this comment

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

Good find

@vb2ae vb2ae merged commit bbb73f0 into Caliburn-Micro:master Nov 11, 2024
4 checks passed
@a44281071
Copy link

Maybe a 'state' like connection is better. Just whether one is activated or not expresses too little meaning.
Pending. InProgress. Completed, Failed...

For example, "In Progress" indicates that it is activating.

For CM.v5, the func name 'activating' and 'Activated/AfterActivated' is better. We don't need 'Active' means it is being activated.
“Don't let the doubt persist”. Just tell us clearly in the tutorial documentation and let us replace it globally.

@vb2ae
Copy link
Member

vb2ae commented Nov 12, 2024

@a44281071 could you create an issue for the requested change?

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.

3 participants