Add support for new method signature of findViewById in Android O #2008

dodgex · 2017-05-26T21:20:37Z

This PR allows to build against Android-O and fixes #2001 by adding an updated version of the view changed notfication code.

naixx · 2017-06-05T19:31:36Z

So, what is the status of the PR?

dodgex · 2017-06-06T12:49:12Z

@WonderCsabo was not yet able to review this PR. Yesterday he told me that he will try to get review this and the other PR asap.

WonderCsabo

Well, we could still optimize the generated code by generating generic findViewById in @EFragment and @EBean. And also remove type casts when finding the views.

I am not sure how much work it would take. The current code should compile on Android O.

WonderCsabo · 2017-06-07T16:51:05Z

...oidannotations-api/src/main/java/org/androidannotations/api/view/OnViewChangedListener2.java

+ */
+package org.androidannotations.api.view;
+
+public interface OnViewChangedListener2 {


Is this class and OnViewChangedNotifier2 necessary? We can create HasViewsBase interface, and let HasViews and HasViews2 extend that. And reference HasViewsBase from OnViewChangedListener and OnViewChangedNotifier.

How could HasViewsBase look like? It would need to have a findViewById with a signature that is compatible to the other two versions. Or do i miss something in your suggestion?

You are right, we call findViewById() on HasViews, for some reason i assumed we just call this.findViewById().

Ah we really should have dropped @EBean view support and the whole HasViews ceremony. :(

Unfortunately yes. So we need this stuff. :/

and do you guys have a name suggestion? (I'm pretty bad in naming stuff)

If you want to use the generic variant, then we must use type cast in the implementing if compileSdkVersion < O. But that would be elegant, we could drop all the individual type casts. So go for it if you think.

Naming is very hard. What about findViewByIdentifier() ?

for now i used internalFindViewById after i asked. if thats ok I'll keep it?

I like it, keep it. 👍

That's most probably won't clash with any user defined findViewById

WonderCsabo · 2017-06-07T16:53:09Z

...roidannotations/src/main/java/org/androidannotations/internal/helper/ViewNotifierHelper.java

 		resetPreviousNotifier(initBlock.blockSimple(), previousNotifier);
 	}

+	private boolean useGenerigFindViewById() {


useGenericFindViewById

WonderCsabo · 2017-06-07T16:54:11Z

...roidannotations/src/main/java/org/androidannotations/internal/helper/ViewNotifierHelper.java

 	}

+	private boolean useGenerigFindViewById() {
+		TypeElement activityType = environment.getProcessingEnvironment().getElementUtils().getTypeElement("android.app.Activity");


string -> CanonicalNameConstants.ACTIVITY

dodgex · 2017-06-10T13:50:05Z

I just updated the PR with a total rewrite of the fix. should be much better now. :)

dodgex · 2017-06-10T13:51:37Z

I have not yet tested it in an Android O build, but for initial review of the changes it should be okay.

WonderCsabo · 2017-06-10T14:11:44Z

...roidannotations/src/main/java/org/androidannotations/internal/helper/ViewNotifierHelper.java

+
+		JVar idParam = findViewById.param(codeModel.INT, "id");
+		IJExpression findViewByIdExpression = holder.getFindViewByIdExpression(idParam);
+		findViewById.body()._return(JExpr.cast(genericType, findViewByIdExpression));


Two things:

type cast is not needed on Android O .

You can remove the cast for other places, since the type will be inferred with this signare

I wanted to not have to check the Android version (or the presence of the generic method) here. Can change that if you want. Not sure if casts have a perf impact?

Done.

Additional Note to 1: It looks like the AppCompatActivity does not yet have the generic findViewById so the cast would be needed on android O + appcompat activity.

i checked the code in android-o-preview-2 and master branches.

Well, it would be nice to gain something from this new signature, if we doing so much work because of it. 😆 Since you already have the check code, i guess you can add it now.

Oh, they are overriding findViewById(), great... Then leave the cast here for now.

i just found that com.android.support:appcompat-v7:26.0.0-beta2 has the new signature.

Cool. Then try to remove the cast with O.

The cast is now gone for Android O. But it now requires to be on appcompat 26.0.0-beta2 (maybe beta1 works too; alpha1 does not).

WonderCsabo · 2017-06-10T14:23:57Z

...annotations/src/main/java/org/androidannotations/holder/EComponentWithViewSupportHolder.java

 	}

+	public IJExpression getFindViewByIdExpression(JVar idParam) {
+		return _null();


Maybe getFindViewByIdExpression should be an abstract method?

I was considering this. Then we'd have the return _null() in EBeanHolder. I decided to keep it this way, but with no solid reason. If you prefer to have this abstract I can change it.

WonderCsabo · 2017-06-10T15:56:11Z

@dodgex it turns out the cast is needed for Fragments even on Android O. 😢

return ((contentView_ == null)?null:contentView_.findViewById(id));

Because using ternary operator and not calling the findViewById() directly, the compiler cannot deduce type T.

dodgex · 2017-06-10T16:10:39Z

@WonderCsabo well while i think it would be nice to not have the cast in the generated code i think the code in AA required to get the cast when we it is needed and not have it when not needed is not worth the benefit. therefore I'd remove the check again and cast always... wdyt?

WonderCsabo · 2017-06-10T16:32:33Z

Okay.

…

On Jun 10, 2017 18:10, "Kay-Uwe Janssen" ***@***.***> wrote: @WonderCsabo <https://github.com/wondercsabo> well while i think it would be nice to not have the cast in the generated code i think the code in AA required to get the cast when we it is needed and not have it when not needed is not worth the benefit. therefore I'd remove the check again and cast always... wdyt? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2008 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdU5dw3YBnsQWsvU3NuheYQE4a7tGivks5sCsAAgaJpZM4NoEnP> .

dodgex · 2017-06-10T16:35:38Z

check is gone. we should be good now =)

WonderCsabo · 2017-06-10T17:39:02Z

I ran the test suite with compileSdkVersion = O, and everything passed.

WonderCsabo · 2017-06-10T17:40:30Z

Nice work, @dodgex !

ghost · 2017-07-24T16:24:15Z

👍

dodgex requested a review from WonderCsabo May 26, 2017 21:20

WonderCsabo suggested changes Jun 7, 2017

View reviewed changes

WonderCsabo suggested changes Jun 10, 2017

View reviewed changes

dodgex added 2 commits June 10, 2017 18:34

Add support for new method signature of findViewById in Android O

b2bc8f6

Remove cast from internalFindViewById() call

516c722

WonderCsabo approved these changes Jun 10, 2017

View reviewed changes

WonderCsabo merged commit 621a577 into androidannotations:develop Jun 10, 2017

WonderCsabo added this to the 4.4 milestone Jun 10, 2017

dodgex deleted the 2001_support_android_o_findviewbyid branch June 10, 2017 17:47

WonderCsabo mentioned this pull request Dec 1, 2017

internalFindViewById raises "unchecked cast" warning #2090

Open

nanchen2251 mentioned this pull request May 13, 2019

update from 3.0.1 to 4.6.0 #2227

Closed

Add support for new method signature of findViewById in Android O #2008

Add support for new method signature of findViewById in Android O #2008

Uh oh!

Conversation

dodgex commented May 26, 2017

Uh oh!

naixx commented Jun 5, 2017

Uh oh!

dodgex commented Jun 6, 2017

Uh oh!

WonderCsabo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodgex Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naixx Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodgex commented Jun 10, 2017

Uh oh!

dodgex commented Jun 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dodgex Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WonderCsabo Jun 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WonderCsabo commented Jun 10, 2017

Uh oh!

dodgex commented Jun 10, 2017

Uh oh!

WonderCsabo commented Jun 10, 2017 via email

Uh oh!

dodgex commented Jun 10, 2017

Uh oh!

WonderCsabo commented Jun 10, 2017

Uh oh!

WonderCsabo commented Jun 10, 2017

dodgex Jun 10, 2017 •

edited

Loading

naixx Jun 10, 2017 •

edited

Loading

dodgex Jun 10, 2017 •

edited

Loading

WonderCsabo Jun 10, 2017 •

edited

Loading