Fix crash when altering rate for voicemails without content.
- Repro for crash was to click on voicemail from call log without
content, try to play it (which would silently fail), then try to
use the rate increase/decrease buttons.
- The error being reported to the ui was disabling all the ui elements
but not the rate buttons.
- This fix makes the ui disable the rate buttons too, as well as showing
a Toast, and logging the exception to logcat.
- This cl also adds the unit tests that prove that the bug is fixed.
- In the process, I discovered another bug where missing extras from
the intent used to start the CallDetailsActivity could cause a crash,
so this adds a test and a fix for that case too.
- Also introduces IntegrationTestUtils class, with some useful methods
for doing things like clicking on different parts of the ui.
Bug: 5047879
Change-Id: I46d18723fe783a7a820446e1e13e19b5af82fa5c
diff --git a/res/values/strings.xml b/res/values/strings.xml
index 54ed931..b90898d 100644
--- a/res/values/strings.xml
+++ b/res/values/strings.xml
@@ -1623,6 +1623,9 @@
<!-- Initial display for position of current playback, do not translate. -->
<string name="voicemail_initial_time">00:05</string>
+ <!-- Message to show when there is an error playing back the voicemail. -->
+ <string name="voicemail_playback_error">Could not play voicemail</string>
+
<!-- The separator between the call type text and the date in the call log [CHAR LIMIT=3] -->
<string name="call_log_type_date_separator">/</string>
diff --git a/src/com/android/contacts/CallDetailActivity.java b/src/com/android/contacts/CallDetailActivity.java
index 2bea2f3..14fec85 100644
--- a/src/com/android/contacts/CallDetailActivity.java
+++ b/src/com/android/contacts/CallDetailActivity.java
@@ -174,13 +174,7 @@
public void onResume() {
super.onResume();
updateData(getCallLogEntryUris());
- Uri voicemailUri = getIntent().getExtras().getParcelable(EXTRA_VOICEMAIL_URI);
- optionallyHandleVoicemail(voicemailUri);
- if (voicemailUri != null) {
- mAsyncQueryHandler.startVoicemailStatusQuery(voicemailUri);
- } else {
- mStatusMessageView.setVisibility(View.GONE);
- }
+ optionallyHandleVoicemail();
}
/**
@@ -189,18 +183,22 @@
* If the Intent used to start this Activity contains the suitable extras, then start voicemail
* playback. If it doesn't, then hide the voicemail ui.
*/
- private void optionallyHandleVoicemail(Uri voicemailUri) {
+ private void optionallyHandleVoicemail() {
+ Uri voicemailUri = getIntent().getParcelableExtra(EXTRA_VOICEMAIL_URI);
FragmentManager manager = getFragmentManager();
VoicemailPlaybackFragment fragment = (VoicemailPlaybackFragment) manager.findFragmentById(
R.id.voicemail_playback_fragment);
- if (voicemailUri == null) {
- // No voicemail uri: hide the voicemail fragment.
- manager.beginTransaction().hide(fragment).commit();
- } else {
- // A voicemail: extra tells us if we should start playback or not.
- boolean startPlayback = getIntent().getExtras().getBoolean(
+ if (voicemailUri != null) {
+ // Has voicemail uri: leave the fragment visible. Optionally start the playback.
+ // Do a query to fetch the voicemail status messages.
+ boolean startPlayback = getIntent().getBooleanExtra(
EXTRA_VOICEMAIL_START_PLAYBACK, false);
fragment.setVoicemailUri(voicemailUri, startPlayback);
+ mAsyncQueryHandler.startVoicemailStatusQuery(voicemailUri);
+ } else {
+ // No voicemail uri: hide the voicemail fragment and the status view.
+ manager.beginTransaction().hide(fragment).commit();
+ mStatusMessageView.setVisibility(View.GONE);
}
}
diff --git a/src/com/android/contacts/CallDetailActivityQueryHandler.java b/src/com/android/contacts/CallDetailActivityQueryHandler.java
index c1d87b2..df5b4f7 100644
--- a/src/com/android/contacts/CallDetailActivityQueryHandler.java
+++ b/src/com/android/contacts/CallDetailActivityQueryHandler.java
@@ -57,8 +57,8 @@
* If the voicemail record does not have an audio yet then it fires the second query to get the
* voicemail status of the associated source.
*/
- public void startVoicemailStatusQuery(Uri voicemaiUri) {
- startQuery(QUERY_VOICEMAIL_CONTENT_TOKEN, null, voicemaiUri, VOICEMAIL_CONTENT_PROJECTION,
+ public void startVoicemailStatusQuery(Uri voicemailUri) {
+ startQuery(QUERY_VOICEMAIL_CONTENT_TOKEN, null, voicemailUri, VOICEMAIL_CONTENT_PROJECTION,
null, null, null);
}
@@ -67,11 +67,12 @@
try {
if (token == QUERY_VOICEMAIL_CONTENT_TOKEN) {
// Query voicemail status only if this voicemail record does not have audio.
- if (cursor.moveToFirst() && hasNoAudio(cursor)) {
+ if (moveToFirst(cursor) && hasNoAudio(cursor)) {
startQuery(QUERY_VOICEMAIL_STATUS_TOKEN, null,
Status.buildSourceUri(getSourcePackage(cursor)),
VoicemailStatusHelperImpl.PROJECTION, null, null, null);
} else {
+ // nothing to show in status
mCallDetailActivity.updateVoicemailStatusMessage(null);
}
} else if (token == QUERY_VOICEMAIL_STATUS_TOKEN) {
@@ -84,6 +85,15 @@
}
}
+ /** Check that the cursor is non-null and can be moved to first. */
+ private boolean moveToFirst(Cursor cursor) {
+ if (cursor == null || !cursor.moveToFirst()) {
+ Log.e(TAG, "Cursor not valid, could not move to first");
+ return false;
+ }
+ return true;
+ }
+
private boolean hasNoAudio(Cursor voicemailCursor) {
return voicemailCursor.getInt(HAS_CONTENT_COLUMN_INDEX) == 0;
}
diff --git a/src/com/android/contacts/voicemail/VoicemailPlaybackFragment.java b/src/com/android/contacts/voicemail/VoicemailPlaybackFragment.java
index 328360b..436f13b 100644
--- a/src/com/android/contacts/voicemail/VoicemailPlaybackFragment.java
+++ b/src/com/android/contacts/voicemail/VoicemailPlaybackFragment.java
@@ -16,21 +16,23 @@
package com.android.contacts.voicemail;
+import com.android.contacts.R;
+import com.android.ex.variablespeed.MediaPlayerProxy;
+import com.android.ex.variablespeed.VariableSpeed;
+
import android.app.Fragment;
import android.content.Context;
import android.media.AudioManager;
import android.net.Uri;
import android.os.Bundle;
+import android.util.Log;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.ImageButton;
import android.widget.SeekBar;
import android.widget.TextView;
-
-import com.android.contacts.R;
-import com.android.ex.variablespeed.MediaPlayerProxy;
-import com.android.ex.variablespeed.VariableSpeed;
+import android.widget.Toast;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -50,6 +52,7 @@
*/
@NotThreadSafe
public class VoicemailPlaybackFragment extends Fragment {
+ private static final String TAG = "VoicemailPlayback";
private static final int NUMBER_OF_THREADS_IN_POOL = 2;
private VoicemailPlaybackPresenter mPresenter;
@@ -217,10 +220,14 @@
}
@Override
- public void playbackError() {
+ public void playbackError(Exception e) {
+ mRateIncreaseButton.setEnabled(false);
+ mRateDecreaseButton.setEnabled(false);
mStartStopButton.setEnabled(false);
mPlaybackSeek.setProgress(0);
mPlaybackSeek.setEnabled(false);
+ Toast.makeText(getActivity(), R.string.voicemail_playback_error, Toast.LENGTH_SHORT);
+ Log.e(TAG, "Could not play voicemail", e);
}
@Override
diff --git a/src/com/android/contacts/voicemail/VoicemailPlaybackPresenter.java b/src/com/android/contacts/voicemail/VoicemailPlaybackPresenter.java
index 11764c9..53e64e9 100644
--- a/src/com/android/contacts/voicemail/VoicemailPlaybackPresenter.java
+++ b/src/com/android/contacts/voicemail/VoicemailPlaybackPresenter.java
@@ -16,6 +16,9 @@
package com.android.contacts.voicemail;
+import com.android.ex.variablespeed.MediaPlayerProxy;
+import com.android.ex.variablespeed.SingleThreadedMediaPlayerProxy;
+
import android.content.Context;
import android.media.MediaPlayer;
import android.net.Uri;
@@ -23,9 +26,6 @@
import android.view.View;
import android.widget.SeekBar;
-import com.android.ex.variablespeed.MediaPlayerProxy;
-import com.android.ex.variablespeed.SingleThreadedMediaPlayerProxy;
-
import java.io.IOException;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
@@ -61,7 +61,7 @@
int getDesiredClipPosition();
void playbackStarted();
void playbackStopped();
- void playbackError();
+ void playbackError(Exception e);
boolean isSpeakerPhoneOn();
void setSpeakerPhoneOn(boolean on);
void finish();
@@ -216,7 +216,7 @@
}
private void handleError(Exception e) {
- mView.playbackError();
+ mView.playbackError(e);
mPlayer.release();
mPositionUpdater.stopUpdating();
}
diff --git a/tests/src/com/android/contacts/CallDetailActivityTest.java b/tests/src/com/android/contacts/CallDetailActivityTest.java
new file mode 100644
index 0000000..a36216e
--- /dev/null
+++ b/tests/src/com/android/contacts/CallDetailActivityTest.java
@@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2011 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.contacts;
+
+import com.android.contacts.util.IntegrationTestUtils;
+import com.google.common.base.Preconditions;
+
+import android.app.Activity;
+import android.content.ContentResolver;
+import android.content.ContentUris;
+import android.content.ContentValues;
+import android.content.Intent;
+import android.net.Uri;
+import android.provider.CallLog;
+import android.test.ActivityInstrumentationTestCase2;
+
+/**
+ * Unit tests for the {@link CallDetailActivity}.
+ */
+public class CallDetailActivityTest extends ActivityInstrumentationTestCase2<CallDetailActivity> {
+ private static final String FAKE_VOICEMAIL_URI_STRING = "content://fake_uri";
+ private Uri mUri;
+ private IntegrationTestUtils mTestUtils;
+
+ public CallDetailActivityTest() {
+ super(CallDetailActivity.class);
+ }
+
+ @Override
+ protected void setUp() throws Exception {
+ super.setUp();
+ // I don't like the default of focus-mode for tests, the green focus border makes the
+ // screenshots look weak.
+ setActivityInitialTouchMode(true);
+ mTestUtils = new IntegrationTestUtils(getInstrumentation());
+ }
+
+ @Override
+ protected void tearDown() throws Exception {
+ cleanUpUri();
+ mTestUtils = null;
+ super.tearDown();
+ }
+
+ /**
+ * Test for bug where increase rate button with invalid voicemail causes a crash.
+ * <p>
+ * The repro steps for this crash were to open a voicemail that does not have an attachment,
+ * then click the play button (which just reported an error), then after that try to adjust the
+ * rate.
+ */
+ public void testClickIncreaseRateButtonWithInvalidVoicemailDoesNotCrash() throws Throwable {
+ setActivityIntentForTestVoicemailEntry();
+ Activity activity = getActivity();
+ mTestUtils.clickButton(activity, R.id.playback_start_stop);
+ mTestUtils.clickButton(activity, R.id.rate_increase_button);
+ }
+
+ /** Test for bug where missing Extras on intent used to start Activity causes NPE. */
+ public void testCallLogUriWithMissingExtrasShouldNotCauseNPE() throws Exception {
+ setActivityIntentForTestCallEntry();
+ getActivity();
+ }
+
+ private void setActivityIntentForTestCallEntry() {
+ createTestCallEntry(false);
+ setActivityIntent(new Intent(Intent.ACTION_VIEW, mUri));
+ }
+
+ private void setActivityIntentForTestVoicemailEntry() {
+ createTestCallEntry(true);
+ Intent intent = new Intent(Intent.ACTION_VIEW, mUri);
+ Uri voicemailUri = Uri.parse(FAKE_VOICEMAIL_URI_STRING);
+ intent.putExtra(CallDetailActivity.EXTRA_VOICEMAIL_URI, voicemailUri);
+ setActivityIntent(intent);
+ }
+
+ /** Inserts an entry into the call log. */
+ private void createTestCallEntry(boolean isVoicemail) {
+ Preconditions.checkState(mUri == null, "mUri should be null");
+ ContentResolver contentResolver = getContentResolver();
+ ContentValues contentValues = new ContentValues();
+ contentValues.put(CallLog.Calls.NUMBER, "01234567890");
+ if (isVoicemail) {
+ contentValues.put(CallLog.Calls.TYPE, CallLog.Calls.VOICEMAIL_TYPE);
+ contentValues.put(CallLog.Calls.VOICEMAIL_URI, FAKE_VOICEMAIL_URI_STRING);
+ } else {
+ contentValues.put(CallLog.Calls.TYPE, CallLog.Calls.INCOMING_TYPE);
+ }
+ contentValues.put(CallLog.Calls.VOICEMAIL_URI, FAKE_VOICEMAIL_URI_STRING);
+ mUri = contentResolver.insert(CallLog.Calls.CONTENT_URI_WITH_VOICEMAIL, contentValues);
+ }
+
+ private void cleanUpUri() {
+ if (mUri != null) {
+ getContentResolver().delete(CallLog.Calls.CONTENT_URI_WITH_VOICEMAIL,
+ "_ID = ?", new String[] { String.valueOf(ContentUris.parseId(mUri)) });
+ mUri = null;
+ }
+ }
+
+ private ContentResolver getContentResolver() {
+ return getInstrumentation().getTargetContext().getContentResolver();
+ }
+}
diff --git a/tests/src/com/android/contacts/util/IntegrationTestUtils.java b/tests/src/com/android/contacts/util/IntegrationTestUtils.java
new file mode 100644
index 0000000..9ce60a0
--- /dev/null
+++ b/tests/src/com/android/contacts/util/IntegrationTestUtils.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2011 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.contacts.util;
+
+import android.app.Activity;
+import android.app.Instrumentation;
+import android.view.View;
+
+import junit.framework.Assert;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.FutureTask;
+
+/** Some utility methods for making integration testing smoother. */
+public class IntegrationTestUtils {
+ private final Instrumentation mInstrumentation;
+
+ public IntegrationTestUtils(Instrumentation instrumentation) {
+ mInstrumentation = instrumentation;
+ }
+
+ /**
+ * Find a view by a given resource id, from the given activity, and click it, iff it is
+ * enabled according to {@link View#isEnabled()}.
+ */
+ public void clickButton(final Activity activity, final int buttonResourceId) throws Throwable {
+ runOnUiThreadAndGetTheResult(new Callable<Void>() {
+ @Override
+ public Void call() throws Exception {
+ View view = activity.findViewById(buttonResourceId);
+ Assert.assertNotNull(view);
+ if (view.isEnabled()) {
+ view.performClick();
+ }
+ return null;
+ }
+ });
+ }
+
+ // TODO: Move this class and the appropriate documentation into a test library, having checked
+ // first to see if exactly this code already exists or not.
+ /**
+ * Execute a callable on the ui thread, returning its result synchronously.
+ * <p>
+ * Waits for an idle sync on the main thread (see {@link Instrumentation#waitForIdle(Runnable)})
+ * before executing this callable.
+ */
+ private <T> T runOnUiThreadAndGetTheResult(Callable<T> callable) throws Throwable {
+ FutureTask<T> future = new FutureTask<T>(callable);
+ mInstrumentation.waitForIdle(future);
+ try {
+ return future.get();
+ } catch (ExecutionException e) {
+ // Unwrap the cause of the exception and re-throw it.
+ throw e.getCause();
+ }
+ }
+}