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

fix: Unloading progressring when loaded #747

Closed
wants to merge 1 commit into from

Conversation

nickrandolph
Copy link
Contributor

GitHub Issue (If applicable): #

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@nickrandolph
Copy link
Contributor Author

@Xiaoy312 @jeromelaban @Youssef1313 @kazo0 would really appreciate some input here. The only way that I've found of unloading the progressring is to set the LoadingContentTemplate to an empty template. The current code (a bit hacky) grabs a local copy of the LoadingContentTemplate so that it can be re-set when the control goes back to the Loading state. Any suggestions on a nicer approach?

@Xiaoy312
Copy link
Contributor

Xiaoy312 commented Aug 22, 2023

I think you can remove the #LoadingContentPresenter from its parent on "Loaded", and then re-add it if it goes back to "Loading". Its content might need to be remove as well, to avoid unnecessarily proc'ing the template resolution & materialization, since the content is still bound to a possibly changing source LoadingContent.

Comment on lines +146 to +147
private DataTemplate? loadingContentTemplatePlaceholder;
private DataTemplateSelector? loadingContentTemplateSelectorPlaceholder;
Copy link
Member

Choose a reason for hiding this comment

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

Should those be reset if the original values change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, and there's probably a bit more refactoring to do. At this stage I just wanted to gauge whether or not there's a better approach before finalising this

Copy link
Member

Choose a reason for hiding this comment

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

Those are private fields, and are not "place holder" but only some "cache" or "backing field". There nothing to reset IMHO.

@nickrandolph
Copy link
Contributor Author

I think you can remove the #LoadingContentPresenter from its parent on "Loaded", and then re-add it if it goes back to "Loading". Its content might need to be remove as well, to avoid unnecessarily proc'ing the template resolution & materialization, since the content is still bound to a possibly changing source LoadingContent.

Will this break the visual states though?

Comment on lines +146 to +147
private DataTemplate? loadingContentTemplatePlaceholder;
private DataTemplateSelector? loadingContentTemplateSelectorPlaceholder;
Copy link
Member

Choose a reason for hiding this comment

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

Those are private fields, and are not "place holder" but only some "cache" or "backing field". There nothing to reset IMHO.

Comment on lines +171 to +172
LoadingContentTemplate = loadingContentTemplatePlaceholder!;
LoadingContentTemplateSelector = loadingContentTemplateSelectorPlaceholder!;
Copy link
Member

Choose a reason for hiding this comment

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

If the user changed the LoadingContentTemplate or the LoadingContentTemplateSelector while the view is "Loaded" they are going to be overwritten.

Copy link
Contributor

@Xiaoy312 Xiaoy312 Aug 22, 2023

Choose a reason for hiding this comment

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

@nickrandolph lets start a thread here, so we can keep the discussion in the same place.

I think you can remove the #LoadingContentPresenter from its parent on "Loaded", and then re-add it if it goes back to "Loading". Its content might need to be remove as well, to avoid unnecessarily proc'ing the template resolution & materialization, since the content is still bound to a possibly changing source LoadingContent.

Will this break the visual states though?

It will if you kept them. But with this approach, you will be dropping the #LoadingContentPresenter setters, to handle it in the code-behind.

@dr1rrb
Copy link
Member

dr1rrb commented Aug 22, 2023

@Xiaoy312 @jeromelaban @Youssef1313 @kazo0 would really appreciate some input here. The only way that I've found of unloading the progressring is to set the LoadingContentTemplate to an empty template. The current code (a bit hacky) grabs a local copy of the LoadingContentTemplate so that it can be re-set when the control goes back to the Loading state. Any suggestions on a nicer approach?

By the time I did a behavior that was walking the visual tree to determine if any parent was Actual<Width|Height>=0, Visibility.Collapsed, Opacity = 0, render transformed our of the viewport. The issue is that it has to poll the visual tree (only when animating) as we don't have any callback for parent's visibility / opacity.

Thinking about it, we now have a much better approach : the EffectiveViewportChanged event! https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.frameworkelement.effectiveviewportchanged?view=winrt-22621

It has the down side that application dev will have to think about it, but the advantage to be usable not only in the LoadingView .

@nickrandolph
Copy link
Contributor Author

@Xiaoy312 @jeromelaban @Youssef1313 @kazo0 would really appreciate some input here. The only way that I've found of unloading the progressring is to set the LoadingContentTemplate to an empty template. The current code (a bit hacky) grabs a local copy of the LoadingContentTemplate so that it can be re-set when the control goes back to the Loading state. Any suggestions on a nicer approach?

By the time I did a behavior that was walking the visual tree to determine if any parent was Actual<Width|Height>=0, Visibility.Collapsed, Opacity = 0, render transformed our of the viewport. The issue is that it has to poll the visual tree (only when animating) as we don't have any callback for parent's visibility / opacity.

Thinking about it, we now have a much better approach : the EffectiveViewportChanged event!

It has the down side that application dev will have to think about it, but the advantage to be usable not only in the LoadingView .

How would we use the EffectiveViewportChanged event? I'm not sure I follow how this would work

@dr1rrb
Copy link
Member

dr1rrb commented Aug 22, 2023

How would we use the EffectiveViewportChanged event? I'm not sure I follow how this would work

This gives you the rectangle that is currently visible for any FrameworkElement. We can create a behavior (or directly package it in the ProgressRing if windows does something similar) that will just stop the animation if the viewport is 0. It should be the case as soon as a parent is Visibility=Collapsed for instance.

@nickrandolph
Copy link
Contributor Author

Replaced by #1002

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.

5 participants