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

Correctly handle ContainerView changes #13114

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented Feb 3, 2023

Description of Change

Issues Fixed

@@ -14,4 +15,11 @@ public interface IViewHandler : IElementHandler

void PlatformArrange(Rect frame);
}

public interface INeedsContainerViewHandler : IViewHandler
Copy link
Member Author

Choose a reason for hiding this comment

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

Ignore this.

Copy link
Member Author

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Who knows if this is good, but it is a WIP.

Comment on lines 228 to 241
public static void MapVisibility(IViewHandler handler, IView view)
{
handler.Invoke(nameof(INeedsContainerViewHandler.NeedsContainer), nameof(IView.Visibility));

if (handler.HasContainer)
{
((PlatformView?)handler.ContainerView)?.UpdateVisibility(view);
((PlatformView?)handler.PlatformView)?.UpdateVisibility(Visibility.Visible);
}
else
{
((PlatformView?)handler.PlatformView)?.UpdateVisibility(view);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

When a property changes that needs to respond to ContainerView changes, we call the "special" invoke and pass the property that requires a re-fire. This updates a collection, and ALSO triggers the path to ensure the container is correct.

Then based on the HasContainer, the mapper needs to set the values of the container and/or the platform view.

Comment on lines 304 to 310
public static void MapNeedsContainer(IViewHandler handler, IView view, object? args)
{
if (handler is ViewHandler viewHandler)
handler.HasContainer = viewHandler.NeedsContainer;
else
handler.HasContainer = view.NeedsContainer();
if (handler is ViewHandler viewHandler && args is string mapper)
viewHandler.ChangesContainer.Add(mapper);

UpdateInputTransparentOnContainerView(handler, view);
handler.UpdateValue(nameof(INeedsContainerViewHandler.NeedsContainer));
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Then the NeedsContainer command adds the incoming property to the list to update later. After that, it calls the NeedsContainer PROPERTY mapper.

Comment on lines 312 to 316
public static void MapNeedsContainer(IViewHandler handler, IView view)
{
#if ANDROID
if (handler.ContainerView is WrapperView wrapper)
wrapper.InputTransparent = view.InputTransparent;
#endif
if (handler is INeedsContainerViewHandler ncvh)
handler.HasContainer = ncvh.NeedsContainer;
}
Copy link
Member Author

@mattleibow mattleibow Feb 3, 2023

Choose a reason for hiding this comment

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

The NeedsContainer property mapper now updates the HasContainer property.

Comment on lines +100 to +101

UpdateValue(nameof(IViewHandler.ContainerView));
Copy link
Member Author

@mattleibow mattleibow Feb 3, 2023

Choose a reason for hiding this comment

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

The HasContainer property adds/removes the container and then calls the ContainerView property mapper.

If the value is the same as before, it is terminated here and we skip the rest.

Comment on lines 318 to 325
public static void MapContainerView(IViewHandler handler, IView view)
{
if (handler is INeedsContainerViewHandler ncvh)
{
foreach (var map in ncvh.ChangesContainer)
handler.UpdateValue(map);
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The ContainerView then re-fires all the things that were added before.

Comment on lines -248 to +305
handler.UpdateValue(nameof(IViewHandler.ContainerView));
(handler as IDynamicContainerViewHandler)?.ContainerAffectingProperties?.Add(nameof(IView.Clip));
Copy link
Member Author

Choose a reason for hiding this comment

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

This new code now adds the property to the collection.

Comment on lines +403 to +408
void OnChangedContainerCollectionChanged(object? sender, EventArgs e) =>
UpdateValue(nameof(IDynamicContainerViewHandler.NeedsContainer));

class CollectionWithEvents : ICollection<string>
{
HashSet<string> properties = new();
Copy link
Member Author

Choose a reason for hiding this comment

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

The collection is special, and knows when a new property has been added. When a new property is added, it will fire the whole container view code.

So if you are adding 2 props that add a container, no need to go through the whole process if the ContainerView is already set - just use it.

Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

LGTM code wise.

@mattleibow mattleibow self-assigned this Mar 8, 2023
@samhouts samhouts added this to the Under Consideration milestone Aug 10, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Sep 11, 2023
@hartez hartez removed their request for review January 10, 2024 20:27
@PureWeen PureWeen added the area-architecture Issues with code structure, SDK structure, implementation details label May 31, 2024
@PureWeen PureWeen removed this from the Under Consideration milestone Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-architecture Issues with code structure, SDK structure, implementation details stale Indicates a stale issue/pr and will be closed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphicsView not shown / issue with IsVisible property
4 participants