-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(android): parity for Ti.UI.View clipMode #14191
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m1ga I tested the PR and it does not work as intended. It does not allow to change property once set. Also it does not work if we remove the clipMode
from the root yellow view
despite that the red view
has it clipMode: disabled
already.
&& nativeView instanceof ViewGroup viewGroup | ||
&& viewGroup.getParent() == null) { | ||
viewGroup.setClipChildren(false); | ||
viewGroup.setClipToPadding(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setClipToPadding
only works if the view has a padding set, which we don't support at this moment.- This may even work unexpected inside ListView.
setClipToChildren
andsetClipToPadding
serves entirely different purposes. - In iOS,
clipsToBounds
is equivalent tosetClipToChildren
, but there's no direct equivalent to Android'ssetClipToPadding
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know. I only used both because of the example module. I can remove the padding one
Correct. Check the readme notes |
Ok, I didn't check the readme notes, but rather compared this property with iOS where it properly works as expected. I believe we should do more testing around it as Android has a pretty different behaviour to both these properties. And yes, it requires to be on top-level view, which actually we can set ourselves in SDK. I suggest that we introduce the |
very good idea! Yeah, I'll do some more testing and adjust the code but thanks for the check and the feedback 👍 |
android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java
Outdated
Show resolved
Hide resolved
android/titanium/src/java/org/appcelerator/titanium/view/TiUIView.java
Outdated
Show resolved
Hide resolved
{ | ||
|
||
if (nativeView instanceof ViewGroup viewGroup) { | ||
if (clipMode == UIModule.CLIP_MODE_DISABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the setClipChildren(clipMode != UIModule.CLIP_MODE_DISABLED)
is fine. Further we can internally set this on top-most view as discussed earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks for that 👍
I've put it into draft as I know that parts are still missing especially the parent part. I've added your other suggestions. Thanks for the review 👍
@@ -1197,6 +1207,14 @@ private void applyCustomBackground(boolean reuseCurrentDrawable) | |||
} | |||
} | |||
|
|||
private void setClipMode(int clipMode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work to get the root view (I haven't tested it though).
private void setClipMode(int clipMode)
{
if (nativeView == null) {
return;
}
View rootView = nativeView;
while (rootView.getParent() != null && rootView instanceof ViewGroup) {
rootView = (View) rootView.getParent();
}
if (rootView instanceof ViewGroup viewGroup) {
viewGroup.setClipChildren(clipMode != CLIP_MODE_DISABLED);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: android.view.ViewRootImpl cannot be cast to android.view.View
think it's one level to high. But I'll do some more tests at another point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the (View)
casting, and declare rootView
as ViewParent
, even or simply keep it as an Object as it's already casted in next if
condition.
Based on https://github.com/mbender74/clipview/tree/main/ti.clipview
Top: this PR with clipping disabled

bottom: default behavior
Run the code and click the first view to toggle:
Ti.UI.CLIP_MODE_DISABLED
andTi.UI.iOS.CLIP_MODE_DISABLED
for backwards compatibility