Replace RoboWindow with PhoneWindow #659

seadowg · 2013-08-22T18:11:24Z

Basic motivation for this was to allow for Activity.getActionBar() to return a real ActionBar instance.

This changes a lot of the code but hopefully benefits Robolectric in a large way as the real Window code is being utilized. There's probably still some work to be done here but we wanted to get a dialog going. Any concerns we have we'll put in line notes on the file changes (Github doesn't let you do this until after posting the request 😕).

This change also requires an extra Android dependency with the compiled PhoneWindow code. This jar would need to be uploaded to org.robolectric on Maven but we currently have a version of the jar with a script for Mavenizing you can grab here.

In other news, this does currently pass all the tests and RobolectricSample runs with this version just fine 😄.

seadowg · 2013-08-22T18:12:02Z

src/main/java/org/robolectric/SdkConfig.java

@@ -39,6 +39,7 @@ public Dependency getSystemResourceDependency() {
        realAndroidDependency("android-base"),
        realAndroidDependency("android-kxml2"),
        realAndroidDependency("android-luni"),
+        realAndroidDependency("android-policy"),


This is the reference to the jar that will need uploaded to Maven to make this work properly.

trevorrjohn · 2013-08-22T19:23:44Z

@buddhistpirate and I can test this out since we are currently trying to run some tests with our ActionBar.

ry4n1m3 · 2013-08-22T22:44:57Z

+1 works for me.

roman-mazur · 2013-08-23T06:33:35Z

src/test/java/org/robolectric/tester/android/view/RoboWindowTest.java

-  }
-
-  @Test
-  public void contentViewShouldBeMeasuredWithSpecExactly() {


As for me this test should be kept somewhere.

Because this change switches out the RoboWindow for the PhoneWindow (which is the same code that runs on the phone) it will use the 'correct' (whatever Android thinks is correct 😉) MeasureSpec. I wrote up a quick modification to the test that doesn't use RoboWindow here.

The test passes but we don't really need to care as PhoneWindow will do the correct thing.

Well, yeah, it makes sense.

roman-mazur · 2013-08-23T07:27:44Z

I'm trying it with AppCompat library and getting

android.util.AndroidRuntimeException: requestFeature() must be called before adding content
    at com.android.internal.policy.impl.PhoneWindow.requestFeature(PhoneWindow.java:215)
    at android.app.Activity.requestWindowFeature(Activity.java:3225)
    at android.support.v7.app.ActionBarActivityDelegateICS.onCreate(ActionBarActivityDelegateICS.java:63)
    at android.support.v7.app.ActionBarActivity.onCreate(ActionBarActivity.java:98)

I do not call setContentView at all though.

seadowg · 2013-08-23T07:42:14Z

@roman-mazur Thanks for trying this out! ❤️

Hmmm. Somehow mContentParent must be being set in the PhoneWindow instance. Possibly the Window set up is happening too early...

What does the test you're running look like exactly?

roman-mazur · 2013-08-23T07:51:26Z

@seadowg Here is an example: https://gist.github.com/roman-mazur/6316618

seadowg · 2013-08-23T08:06:34Z

@roman-mazur I can't see anything immediately obvious... We'll have a look at it tomorrow. Thanks!

seadowg · 2013-08-23T19:04:00Z

@roman-mazur we believe we've fixed the problem with feature requests. Would you be able to pull the change in and try it if you get time?

It seems that we have to delay setting up the Activity with it's mDecor reference until after the onCreate code has run. This does mean that you can no longer test dialogs (or anything that adds views to the window) without an Activity that calls setContentView. This mimics the real world though so it shouldn't be a problem.

JakeWharton · 2013-08-23T19:24:41Z

If memory serves, onPostCreate calls initActionBar which will force decor view initialization if it hasn't already occurred. So provided you are using your activity builder to step an activity up through onResume it should be fine.

trevorrjohn · 2013-08-23T20:29:16Z

Should we try this again?

seadowg · 2013-08-23T20:47:22Z

@tjohn your issue seems to be caused by another problem in Robolectric that the PhoneWindow exposes (by calling onCreateOptionsMenu). Action views are not being inflated from the MenuInflater in Robolectric. We'll file that separately.

seadowg · 2013-08-23T20:58:33Z

@JakeWharton Yeah that seems to be true but Android weirdly doesn't add the decor view to the Activity's WindowManager instance. We discovered that Views can't be clicked on (using a checked click) unless this happens as it won't be 'shown' as the view hierarchy won't have a ViewRoot as a parent.

Currently we're setting mDecor on the Activity and calling makeVisible in ActivityController.create to set up the root correctly. We're looking into a better way to do this this afternoon.

seadowg · 2013-08-24T00:33:05Z

src/main/java/org/robolectric/util/ActivityController.java

@@ -178,6 +183,18 @@ public void run() {
    return this;
  }

+  public ActivityController<T> visible() {


This needs to be called in tests that deal with View.isShown. It operation could happen at the end of postResume but I feel this is more explicit (and mirrors real life more accurately as it really happens sometime after postResume when the Activity is on screen).

This does make everything quite verbose though. Maybe there needs to be a simpler helper that runs through the whole initialization life cycle (including visible)?

Maybe there needs to be a simpler helper that runs through the whole initialization life cycle (including visible)?

Yes. Sounds reasonable and useful.

Another thing to consider is that the lifecycle is a state machine. Perhaps this class should mirror that state machine more closely by playing any missing steps automatically. This way, calling something like destroy() would automatically hit onPause, onStop, and then onDestroy.

roman-mazur · 2013-08-24T06:41:58Z

@seadowg I'll be able to try it again next week only.

Actually in real world I may add views to a window without calling setContentView. I can add a fragment to android.R.id.content.

activity.getSupportFragmentManager()
    .beginTransaction()
    .add(android.R.id.content, new MyFragment())
    .commit();

mag · 2013-08-26T01:01:58Z

This looks promising! It's a big change though. Seems like we should try to get this code into as many Robolectric 2.0 projects as we can, to be sure that it doesn't break people in weird ways. @seadowg, would it be worthwhile to post a message to the Robolectric google group to see if more people will try this fork and give feedback?

seadowg · 2013-08-26T16:59:32Z

@roman-mazur Thanks! Adding a fragment like this will work as android.R.id.content will exist whether you call setContentView or not. From trying it out it seems that you are correct and setContentView is not required unless you explicitly need it.

@mag Good idea! We'll post a message there.

roman-mazur · 2013-08-28T15:58:01Z

@seadowg I've tried it once more.
But Activity.getActionBar() doesn't seem to work for me returning null.
I've updated the gist https://gist.github.com/roman-mazur/6316618 with 2 cases crucial for tests in my current project.

Here is the error stacktrace I'm getting

java.lang.NullPointerException
    at android.support.v7.app.ActionBarImplICS.setDisplayHomeAsUpEnabled(ActionBarImplICS.java:161)
    at com.myapp.MyActivity.onCreate(MyActivity.java:150)
    at android.app.Activity.performCreate(Activity.java:5008)
    at org.fest.reflect.method.Invoker.invoke(Invoker.java:112)
    at org.robolectric.util.ActivityController$1.run(ActivityController.java:124)
    at org.robolectric.shadows.ShadowLooper.runPaused(ShadowLooper.java:256)
    at org.robolectric.util.ActivityController.create(ActivityController.java:118)
    at org.robolectric.util.ActivityController.create(ActivityController.java:131)

mag · 2013-08-29T16:42:17Z

Hi Roman,

Thanks a lot for documenting this problem. We (@coreydowning and I) spent some time looking at this yesterday.

Unfortunately this pull request isn't working correctly with AppCompat. Here's our naive understanding of why:

The base Theme used by AppCompat sets android:windowNoTitle to 'true'. According to the comment on line 94 of themes_base.xml:

PhoneWindow sees this attribute and decides there is no ActionBar...and whatever is supposed to put it back is not happening (or at least, not at the right time).

We haven't quite figured out what the appropriate approach to this would be. We believe that this pull request shouldn't be blocked by an incompatibility with AppCompat, as you will be no worse off with this change than you were before. However, if we get this through, a new issue should be raised to flag this problem.

coreydowning · 2013-08-29T21:27:47Z

OK, pushing up the rebase now.

Also looking into pushing this to sonatype which sounds reasonable on our end.

mag · 2013-08-30T00:24:36Z

In other news: @coreydowning and I ran some loose experiments with one of our internal projects, and this branch actually ran our tests a bit faster (~10 seconds faster on ~1.25 minute test suite). So while that's just one project, we are confident this doesn't slow things down much if at all.

…of RoboWindowManager

@implementation

…to it (with getViews) The Shadow calls the real object in the @implementation methods so no reality is lost.

…s might break realism but makes everything work)

…g errors in PhoneWindow.measure().

…maven central.

…vity create This also makes the ActionBar feature work by default which may be problematic for older applications.

This means that ActivityController.create now has to set up the Activity window to be visible

This value needs to be set to the Window's decor view and this can not be accessed until after onCreate is called or feature requests will fail within onCreate.

This just switches the tests to use a View that follows a standard lifecycle rather than setting the contentView after resuming.

…visible This method adds the Activity's window decor view to the window manager. This is required for `View.isShown` to return true as otherwise the Activity's view hierarchy won't have a parent. In real life this happens some time after the Activity resumes (in `ActivityThread`) so it really needs to be its own lifecycle operation (even though its not part of the 'official' lifecycle).

Conflicts: .pairs src/main/java/org/robolectric/shadows/ShadowActivity.java src/main/java/org/robolectric/util/ActivityController.java Also removed `ShadowActivity#setContentView` because we can use the real one now 😄

This allows the phone-window branch to work without manually mavenizing the Android policy jar. This should probably be removed once the policy jar is in maven central. :sunglasses:

seadowg · 2013-08-30T21:39:23Z

@coreydowning 😆 Travis passing? Nice one guys!

JakeWharton · 2013-08-30T21:40:32Z

👍 Good to merge then?

mag · 2013-08-30T21:46:40Z

@JakeWharton Sure, go for it. We think this is pretty safe at this point and no major objections have been raised in the community.

@coreydowning says SHIP IT 🌋

JakeWharton · 2013-08-30T21:50:10Z

Replace RoboWindow with PhoneWindow

seadowg · 2013-08-30T22:06:36Z

@JakeWharton @mag @coreydowning🔥🍻🎉🤘

coreydowning · 2013-08-30T23:21:08Z

... ☁️ 🔌 🐑 ❓

JurgenCruz · 2013-09-24T04:33:56Z

Any news as to when will AppCompat will be compatible with Robolectric 2.2? I have been trying to make my 2.1 tests to pass with 2.2 but they simple wot pass.

What is happening in my tests is that the Theme.AppCompat is being set but when ActionBarActivityDelegate runs onCreate the first thing it does is to get obtainStyledAttributes(R.styleable.ActionBarWindow); which for some reason returns a set full of zeros and because of this, ActionBarActivityDelegate thinks the wrong theme is set and crashes.

I've been debuging the latest master and it seems that the method ShadowResources.attrsToTypedArray can't handle getting the attributes for the Theme.AppCompat.

Not sure what is the problem, even debugging I don't know enough of robolectrics internals to find the issue. And God is it hard to debug with the custom class loaders and shadows popping every single step XD.

seadowg reviewed Aug 22, 2013
View reviewed changes

roman-mazur reviewed Aug 23, 2013
View reviewed changes

seadowg reviewed Aug 24, 2013
View reviewed changes

coreydowning mentioned this pull request Aug 27, 2013

Remove lifecycle code and warnings from constructor in ShadowActivity #663

Merged

Callum Stott and Mike Grafton and others added 16 commits August 30, 2013 13:51

Use PhoneWindow instead of RoboWindow, and WindowManagerImpl instead …

b08cfa1

…of RoboWindowManager

Add ShadowWindowManagerImpl to allow for which views have been added …

d70aeae

…to it (with getViews) The Shadow calls the real object in the @implementation methods so no reality is lost.

Allow Converter to handle 'dimen' resources for the moment (think thi…

4425e58

…s might break realism but makes everything work)

Add Corey Downing to pairs

9b98353

Use real Dialog/AlertDialog code for show(). The fake code was causin…

955116a

…g errors in PhoneWindow.measure().

Add android-policy jar to dependencies. This needs to be uploaded to …

671d36c

…maven central.

Clean up Window instantiation in ShadowActivity

5d0f2e0

Make sure decorView for Activity windows are set up correctly on Acti…

07422a6

…vity create This also makes the ActionBar feature work by default which may be problematic for older applications.

Remove RoboWindow and move helpers to ShadowWindow

714ce45

This means that ActivityController.create now has to set up the Activity window to be visible

Alphabetize Shadows

6c36b35

Actually delete RoboWindow

505cf82

Don't set mDecor on Activity until after create

6f9c969

This value needs to be set to the Window's decor view and this can not be accessed until after onCreate is called or feature requests will fail within onCreate.

Update View attaching tests to be more realistic

21d658a

This just switches the tests to use a View that follows a standard lifecycle rather than setting the contentView after resuming.

Rebase master into phone-window

800afc9

Conflicts: .pairs src/main/java/org/robolectric/shadows/ShadowActivity.java src/main/java/org/robolectric/util/ActivityController.java Also removed `ShadowActivity#setContentView` because we can use the real one now 😄

Add Sonatype repository for resolving real android dependencies.

e292611

This allows the phone-window branch to work without manually mavenizing the Android policy jar. This should probably be removed once the policy jar is in maven central. :sunglasses:

JakeWharton added a commit that referenced this pull request Aug 30, 2013

Merge pull request #659 from seadowg/phone-window

3fccd57

Replace RoboWindow with PhoneWindow

JakeWharton merged commit 3fccd57 into robolectric:master Aug 30, 2013

mag mentioned this pull request Sep 8, 2013

Today's 2.2-SNAPSHOT update broke my tests :( #671

Closed

thorikawa mentioned this pull request Sep 20, 2013

Fix custom title issue #696

Merged

coreydowning mentioned this pull request Sep 30, 2013

getActionBar does not work when using AppCompat lib #724

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace RoboWindow with PhoneWindow #659

Replace RoboWindow with PhoneWindow #659

seadowg commented Aug 22, 2013

seadowg Aug 22, 2013

trevorrjohn commented Aug 22, 2013

ry4n1m3 commented Aug 22, 2013

roman-mazur Aug 23, 2013

seadowg Aug 23, 2013

roman-mazur Aug 23, 2013

roman-mazur commented Aug 23, 2013

seadowg commented Aug 23, 2013

roman-mazur commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg commented Aug 23, 2013

JakeWharton commented Aug 23, 2013

trevorrjohn commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg Aug 24, 2013

JakeWharton Aug 29, 2013

roman-mazur commented Aug 24, 2013

mag commented Aug 26, 2013

seadowg commented Aug 26, 2013

roman-mazur commented Aug 28, 2013

mag commented Aug 29, 2013

coreydowning commented Aug 29, 2013

mag commented Aug 30, 2013

seadowg commented Aug 30, 2013

JakeWharton commented Aug 30, 2013

mag commented Aug 30, 2013

JakeWharton commented Aug 30, 2013

seadowg commented Aug 30, 2013

coreydowning commented Aug 30, 2013

JurgenCruz commented Sep 24, 2013

Replace RoboWindow with PhoneWindow #659

Replace RoboWindow with PhoneWindow #659

Conversation

seadowg commented Aug 22, 2013

seadowg Aug 22, 2013

Choose a reason for hiding this comment

trevorrjohn commented Aug 22, 2013

ry4n1m3 commented Aug 22, 2013

roman-mazur Aug 23, 2013

Choose a reason for hiding this comment

seadowg Aug 23, 2013

Choose a reason for hiding this comment

roman-mazur Aug 23, 2013

Choose a reason for hiding this comment

roman-mazur commented Aug 23, 2013

seadowg commented Aug 23, 2013

roman-mazur commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg commented Aug 23, 2013

JakeWharton commented Aug 23, 2013

trevorrjohn commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg commented Aug 23, 2013

seadowg Aug 24, 2013

Choose a reason for hiding this comment

JakeWharton Aug 29, 2013

Choose a reason for hiding this comment

roman-mazur commented Aug 24, 2013

mag commented Aug 26, 2013

seadowg commented Aug 26, 2013

roman-mazur commented Aug 28, 2013

mag commented Aug 29, 2013

coreydowning commented Aug 29, 2013

mag commented Aug 30, 2013

seadowg commented Aug 30, 2013

JakeWharton commented Aug 30, 2013

mag commented Aug 30, 2013

JakeWharton commented Aug 30, 2013

seadowg commented Aug 30, 2013

coreydowning commented Aug 30, 2013

JurgenCruz commented Sep 24, 2013