Revert #24222 and simply use invalidate instead of postInvalidate
#24990
Conversation
|
Hey there @albyrock87! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
rmarinho
requested review from
jonathanpeppers
and removed request for
Eilon
| * @param hasClip | ||
| */ | ||
| protected final void setHasClip(boolean hasClip) { | ||
| if (this.hasClip != hasClip) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
ifis kind-of useless - this method is being called from the C# layer when the
Clipchanges, that's why we always need to invalidate- calling it from C# would cause two JNI roundtrips for nothing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Because the clip path or the shadow details like color, radius,... have changed.
There was a problem hiding this comment.
we should call these methods something like invalidateShadow(bool willDrawShadow) but it's a public API and I can't do that
There was a problem hiding this comment.
If the clip path or shadow changes, should those be the things that call invalidate()?
There was a problem hiding this comment.
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.
| * @param hasShadow | ||
| */ | ||
| protected final void setHasShadow(boolean hasShadow) { | ||
| if (this.hasShadow != hasShadow) { |
There was a problem hiding this comment.
Same answer here :D
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
LGTM, will include the new aar as part of this PR. Waiting build to complete. |
|
/backport to release/8.0.1xx-sr9 |
|
Started backporting to release/8.0.1xx-sr9: https://github.com/dotnet/maui/actions/runs/11221159064 |
jsuarezruiz
dismissed stale reviews from jonathanpeppers and themself
via
5b05cea
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
github-actions
bot
force-pushed
the
revert-has-shadow-has-clip
branch
from
5b05cea to
84c7791
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
github-actions
bot
force-pushed
the
revert-has-shadow-has-clip
branch
from
84c7791 to
c581c9d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
samhouts
added
the
fixed-in-net8.0-nightly
samhouts
added
fixed-in-9.0.10
fixed-in-net8.0-nightly
Description of Change
#24222 broke reaction to shadow / clip changes.
This PR brings back the original code while replacing
postInvalidatewithinvalidateto avoid countless scheduled invalidations considering that MAUI only operates on platform views through the main thread.You can see more details in the issue's comments.
While CI regenerates the
aar, we should commit the newaaronce this is merged, or as part of this PR.I've not included it for security reasons.
Issues Fixed
Fixes #24882