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.

Sign up for GitHub

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

Jump to bottom

Revert #24222 and simply use invalidate instead of postInvalidate #24990

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Revert #24222 and simply use invalidate instead of postInvalidate #24990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5656f4d
Revert #24222 and simply use `invalidate` instead of `postInvalidate`
albyrock87 Sep 29, 2024
c581c9d
Updated maui.aar
jsuarezruiz Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformContentViewGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,12 @@ public PlatformContentViewGroup(Context context, AttributeSet attrs, int defStyl

/**
* Set by C#, determining if we need to call getClipPath()
* Intentionally invalidates the view in case clip changed
* @param hasClip
*/
protected final void setHasClip(boolean hasClip) {
if (this.hasClip != hasClip) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we remove the if? If the value for hasClip hasn't changed, why does it need to run invalidate()?

Does it actually need to be called in a different place in the stack? In .NET MAUI's C# layer, perhaps it would know what changed?

Copy link
Contributor Author

@albyrock87 albyrock87 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jonathanpeppers I've just done what you asked: revert your PR and use invalidate instead of postInvalidate.

That said:

  • setting a boolean field value has very likely no impact on performance, so the if is kind-of useless
  • this method is being called from the C# layer when the Clip changes, that's why we always need to invalidate
    • calling it from C# would cause two JNI roundtrips for nothing

Copy link
Member

Choose a reason for hiding this comment

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

setting a boolean field value has very likely no impact on performance

Setting the field isn't the question, but the fact that invalidate() is called. Why do we need call invalidate() if true is set to true? or false set to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the clip path or the shadow details like color, radius,... have changed.

Copy link
Contributor Author

@albyrock87 albyrock87 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we should call these methods something like invalidateShadow(bool willDrawShadow) but it's a public API and I can't do that

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 clip path or shadow changes, should those be the things that call invalidate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to not move invalidate call there is just to avoid two round trips on JNI. So this is just a naming problem in my opinion.

this.hasClip = hasClip;
postInvalidate();
}
this.hasClip = hasClip;
invalidate();
}

@Override
Expand Down
7 changes: 3 additions & 4 deletions src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformWrapperView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,12 @@ public PlatformWrapperView(Context context) {

/**
* Set by C#, determining if we need to call drawShadow()
* Intentionally invalidates the view in case shadow definition changed
* @param hasShadow
*/
protected final void setHasShadow(boolean hasShadow) {
if (this.hasShadow != hasShadow) {
Copy link
Member

Choose a reason for hiding this comment

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

Same question here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer here :D

this.hasShadow = hasShadow;
postInvalidate();
}
this.hasShadow = hasShadow;
invalidate();
}

@Override
Expand Down
Binary file modified src/Core/src/maui.aar
View file
Open in desktop
Binary file not shown.
Loading