Updated logic on showing voice/video/sms options in favorites menus.
Bug: 78492066
Test: numerous
PiperOrigin-RevId: 194635336
Change-Id: I7be0efad4dc9e11beceb02c9b2f4c719d29dbbd1
diff --git a/java/com/android/dialer/speeddial/ContextMenu.java b/java/com/android/dialer/speeddial/ContextMenu.java
index 126373c..09505ab 100644
--- a/java/com/android/dialer/speeddial/ContextMenu.java
+++ b/java/com/android/dialer/speeddial/ContextMenu.java
@@ -18,10 +18,12 @@
import android.content.Context;
import android.support.annotation.Nullable;
+import android.support.annotation.VisibleForTesting;
import android.util.AttributeSet;
import android.view.View;
import android.widget.LinearLayout;
import android.widget.TextView;
+import com.android.dialer.common.Assert;
import com.android.dialer.speeddial.database.SpeedDialEntry.Channel;
import com.android.dialer.speeddial.loader.SpeedDialUiItem;
@@ -30,13 +32,13 @@
private ContextMenuItemListener listener;
+ private TextView voiceView;
private TextView videoView;
private TextView smsView;
private SpeedDialUiItem speedDialUiItem;
private Channel voiceChannel;
private Channel videoChannel;
- private Channel smsChannel;
public ContextMenu(Context context, @Nullable AttributeSet attrs) {
super(context, attrs);
@@ -50,9 +52,11 @@
videoView.setOnClickListener(v -> placeVideoCall());
smsView = findViewById(R.id.send_message_container);
- smsView.setOnClickListener(v -> listener.openSmsConversation(smsChannel.number()));
+ smsView.setOnClickListener(v -> listener.openSmsConversation(voiceChannel.number()));
- findViewById(R.id.voice_call_container).setOnClickListener(v -> placeVoiceCall());
+ voiceView = findViewById(R.id.voice_call_container);
+ voiceView.setOnClickListener(v -> placeVoiceCall());
+
findViewById(R.id.remove_container)
.setOnClickListener(v -> listener.removeFavoriteContact(speedDialUiItem));
findViewById(R.id.contact_info_container)
@@ -76,14 +80,11 @@
setX((float) (childLocation[0] + .5 * childLayout.getWidth() - .5 * getWidth()));
setY(childLocation[1] - parentLocation[1] + childLayout.getHeight());
- voiceChannel = speedDialUiItem.getDeterministicVoiceChannel();
- videoChannel = speedDialUiItem.getDeterministicVideoChannel();
- videoView.setVisibility(
- videoChannel == null && !speedDialUiItem.hasVideoChannels() ? View.GONE : View.VISIBLE);
-
- // TODO(calderwoodra): disambig dialog for texts?
- smsChannel = voiceChannel;
- smsView.setVisibility(smsChannel == null ? View.GONE : View.VISIBLE);
+ voiceChannel = speedDialUiItem.getDefaultVoiceChannel();
+ videoChannel = speedDialUiItem.getDefaultVideoChannel();
+ voiceView.setVisibility(videoChannel == null ? View.GONE : View.VISIBLE);
+ videoView.setVisibility(videoChannel == null ? View.GONE : View.VISIBLE);
+ smsView.setVisibility(voiceChannel == null ? View.GONE : View.VISIBLE);
// TODO(calderwoodra): a11y
// TODO(calderwoodra): animate this similar to the bubble menu
@@ -102,19 +103,11 @@
}
private void placeVoiceCall() {
- if (voiceChannel == null) {
- listener.disambiguateCall(speedDialUiItem);
- } else {
- listener.placeCall(voiceChannel);
- }
+ listener.placeCall(Assert.isNotNull(voiceChannel));
}
private void placeVideoCall() {
- if (videoChannel == null) {
- listener.disambiguateCall(speedDialUiItem);
- } else {
- listener.placeCall(videoChannel);
- }
+ listener.placeCall(Assert.isNotNull(videoChannel));
}
public boolean isVisible() {
@@ -122,19 +115,12 @@
}
/** Listener to report user clicks on menu items. */
+ @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
public interface ContextMenuItemListener {
/** Called when the user selects "voice call" or "video call" option from the context menu. */
void placeCall(Channel channel);
- /**
- * Called when the user selects "voice call" or "video call" option from the context menu, but
- * it's not clear which channel they want to call.
- *
- * <p>TODO(calderwoodra): discuss with product how we want to handle these cases
- */
- void disambiguateCall(SpeedDialUiItem speedDialUiItem);
-
/** Called when the user selects "send message" from the context menu. */
void openSmsConversation(String number);
diff --git a/java/com/android/dialer/speeddial/SpeedDialFragment.java b/java/com/android/dialer/speeddial/SpeedDialFragment.java
index 54709e4..97a5fac 100644
--- a/java/com/android/dialer/speeddial/SpeedDialFragment.java
+++ b/java/com/android/dialer/speeddial/SpeedDialFragment.java
@@ -130,7 +130,6 @@
contextMenuBackground,
new SpeedDialContextMenuItemListener(
getActivity(),
- getChildFragmentManager(),
new UpdateSpeedDialAdapterListener(),
speedDialLoaderListener),
layoutManager);
@@ -321,19 +320,17 @@
Channel defaultChannel = speedDialUiItem.defaultChannel();
// Add voice call module
- Channel voiceChannel = speedDialUiItem.getDeterministicVoiceChannel();
+ Channel voiceChannel = speedDialUiItem.getDefaultVoiceChannel();
if (voiceChannel != null) {
modules.add(
IntentModule.newCallModule(
getContext(),
new CallIntentBuilder(voiceChannel.number(), CallInitiationType.Type.SPEED_DIAL)
.setAllowAssistedDial(true)));
- } else {
- modules.add(new DisambigDialogModule(speedDialUiItem, /* isVideo = */ false));
}
// Add video if we can determine the correct channel
- Channel videoChannel = speedDialUiItem.getDeterministicVideoChannel();
+ Channel videoChannel = speedDialUiItem.getDefaultVideoChannel();
if (videoChannel != null) {
modules.add(
IntentModule.newCallModule(
@@ -341,8 +338,6 @@
new CallIntentBuilder(videoChannel.number(), CallInitiationType.Type.SPEED_DIAL)
.setIsVideoCall(true)
.setAllowAssistedDial(true)));
- } else if (speedDialUiItem.hasVideoChannels()) {
- modules.add(new DisambigDialogModule(speedDialUiItem, /* isVideo = */ true));
}
// Add sms module
@@ -400,68 +395,24 @@
return false;
}
}
-
- private final class DisambigDialogModule implements HistoryItemActionModule {
-
- private final SpeedDialUiItem speedDialUiItem;
- private final boolean isVideo;
-
- DisambigDialogModule(SpeedDialUiItem speedDialUiItem, boolean isVideo) {
- this.speedDialUiItem = speedDialUiItem;
- this.isVideo = isVideo;
- }
-
- @Override
- public int getStringId() {
- if (isVideo) {
- return R.string.contact_menu_video_call;
- } else {
- return R.string.contact_menu_voice_call;
- }
- }
-
- @Override
- public int getDrawableId() {
- if (isVideo) {
- return R.drawable.quantum_ic_videocam_vd_theme_24;
- } else {
- return R.drawable.quantum_ic_phone_vd_theme_24;
- }
- }
-
- @Override
- public boolean onClick() {
- DisambigDialog.show(speedDialUiItem, getChildFragmentManager());
- return true;
- }
- }
}
private static final class SpeedDialContextMenuItemListener implements ContextMenuItemListener {
private final FragmentActivity activity;
- private final FragmentManager childFragmentManager;
private final SupportUiListener<ImmutableList<SpeedDialUiItem>> speedDialLoaderListener;
private final UpdateSpeedDialAdapterListener updateAdapterListener;
SpeedDialContextMenuItemListener(
FragmentActivity activity,
- FragmentManager childFragmentManager,
UpdateSpeedDialAdapterListener updateAdapterListener,
SupportUiListener<ImmutableList<SpeedDialUiItem>> speedDialLoaderListener) {
this.activity = activity;
- this.childFragmentManager = childFragmentManager;
this.updateAdapterListener = updateAdapterListener;
this.speedDialLoaderListener = speedDialLoaderListener;
}
@Override
- public void disambiguateCall(SpeedDialUiItem speedDialUiItem) {
- // TODO(calderwoodra): show only video or voice channels in the disambig dialog
- DisambigDialog.show(speedDialUiItem, childFragmentManager);
- }
-
- @Override
public void placeCall(Channel channel) {
if (channel.technology() == Channel.DUO) {
Logger.get(activity)
diff --git a/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java b/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java
index c5a3ea3..325af23 100644
--- a/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java
+++ b/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java
@@ -162,61 +162,77 @@
}
/**
- * Returns a video channel if there is exactly one video channel or the default channel is a video
- * channel.
+ * Returns one of the following:
+ *
+ * <ul>
+ * <li>The default channel if it's a video channel.
+ * <li>A video channel if it has the same attributes as the default channel, OR
+ * <li>null. (This is a deliberate product decision, even if there is only a single video
+ * reachable channel, we should still return null if it has different attributes from those
+ * in the default channel).
+ * </ul>
*/
@Nullable
- public Channel getDeterministicVideoChannel() {
- if (defaultChannel() != null && defaultChannel().isVideoTechnology()) {
+ public Channel getDefaultVideoChannel() {
+ if (defaultChannel() == null) {
+ return null;
+ }
+
+ if (defaultChannel().isVideoTechnology()) {
return defaultChannel();
}
- Channel videoChannel = null;
- for (Channel channel : channels()) {
- if (channel.isVideoTechnology()) {
- if (videoChannel != null) {
- // We found two video channels, so we can't determine which one is correct..
- return null;
- }
- videoChannel = channel;
- }
+ if (channels().size() == 1) {
+ // If there is only a single channel, it can't be a video channel
+ return null;
}
- // Only found one channel, so return it
- return videoChannel;
- }
- /** Returns true if any channels are video channels. */
- public boolean hasVideoChannels() {
- for (Channel channel : channels()) {
- if (channel.isVideoTechnology()) {
- return true;
+ // At this point, the default channel is a *voice* channel and there are more than
+ // one channel in total.
+ //
+ // Our defined assumptions about the channel list include that if a video channel
+ // follows a voice channel, it has the same attributes as that voice channel
+ // (see comments on method channels() for details).
+ //
+ // Therefore, if the default video channel exists, it must be the immediate successor
+ // of the default channel in the list.
+ //
+ // Note that we don't have to check if the last channel in the list is the default
+ // channel because even if it is, there will be no video channel under the assumption
+ // above.
+ for (int i = 0; i < channels().size() - 1; i++) {
+ // Find the default channel
+ if (Objects.equals(defaultChannel(), channels().get(i))) {
+ // Our defined assumptions about the list of channels is that if a video channel follows a
+ // voice channel, it has the same attributes as that voice channel.
+ Channel channel = channels().get(i + 1);
+ if (channel.isVideoTechnology()) {
+ return channel;
+ }
+ // Since the default voice channel isn't video reachable, we can't video call this number
+ return null;
}
}
- return false;
+ throw Assert.createIllegalStateFailException("channels() doesn't contain defaultChannel().");
}
/**
- * Returns a voice channel if there is exactly one voice channel or the default channel is a voice
+ * Returns a voice channel if there is exactly one channel or the default channel is a voice
* channel.
*/
@Nullable
- public Channel getDeterministicVoiceChannel() {
+ public Channel getDefaultVoiceChannel() {
if (defaultChannel() != null && !defaultChannel().isVideoTechnology()) {
return defaultChannel();
}
- Channel voiceChannel = null;
- for (Channel channel : channels()) {
- if (!channel.isVideoTechnology()) {
- if (voiceChannel != null) {
- // We found two voice channels, so we can't determine which one is correct..
- return null;
- }
- voiceChannel = channel;
- }
+ if (channels().size() == 1) {
+ // If there is only a single channel, it must be a voice channel as per our defined
+ // assumptions (detailed in comments on method channels()).
+ return channels().get(0);
}
- // Only found one channel, so return it
- return voiceChannel;
+
+ return null;
}
/**
@@ -254,6 +270,21 @@
* enumerate each one here so that the user can choose the correct one. Each channel here
* represents a row in the {@link com.android.dialer.speeddial.DisambigDialog}.
*
+ * <p>These channels have a few very strictly enforced assumption that are used heavily throughout
+ * the codebase. Those assumption are that:
+ *
+ * <ol>
+ * <li>Each of the contact's numbers are voice reachable. So if a channel has it's technology
+ * set to anything other than {@link Channel#VOICE}, there is gaurenteed to be another
+ * channel with the exact same attributes, but technology will be {@link Channel#VOICE}.
+ * <li>For each of the contact's phone numbers, there will be a voice channel, then the next
+ * channel will either be the same phone number but a video channel, or a new number.
+ * </ol>
+ *
+ * For example: Say a contact has two phone numbers (A & B) and A is duo reachable. Then you can
+ * assume the list of channels will be ordered as either {A_voice, A_duo, B_voice} or {B_voice,
+ * A_voice, A_duo}.
+ *
* @see com.android.dialer.speeddial.database.SpeedDialEntry.Channel
*/
public abstract ImmutableList<Channel> channels();