Updated logic on showing voice/video/sms options in favorites menus.

Bug: 78492066
Test: numerous
PiperOrigin-RevId: 194635336
Change-Id: I7be0efad4dc9e11beceb02c9b2f4c719d29dbbd1

3 files changed, 83 insertions, 115 deletions

diff --git a/java/com/android/dialer/speeddial/ContextMenu.java b/java/com/android/dialer/speeddial/ContextMenu.java
index 126373c12..09505ab99 100644
--- a/java/com/android/dialer/speeddial/ContextMenu.java
+++ b/java/com/android/dialer/speeddial/ContextMenu.java
@@ -18,10 +18,12 @@ package com.android.dialer.speeddial;
 
 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 @@ public class ContextMenu extends LinearLayout {
 
   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 @@ public class ContextMenu extends LinearLayout {
     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()));
+
+    voiceView = findViewById(R.id.voice_call_container);
+    voiceView.setOnClickListener(v -> placeVoiceCall());
 
-    findViewById(R.id.voice_call_container).setOnClickListener(v -> placeVoiceCall());
     findViewById(R.id.remove_container)
         .setOnClickListener(v -> listener.removeFavoriteContact(speedDialUiItem));
     findViewById(R.id.contact_info_container)
@@ -76,14 +80,11 @@ public class ContextMenu extends LinearLayout {
     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 @@ public class ContextMenu extends LinearLayout {
   }
 
   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 @@ public class ContextMenu extends LinearLayout {
   }
 
   /** 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 54709e491..97a5facab 100644
--- a/java/com/android/dialer/speeddial/SpeedDialFragment.java
+++ b/java/com/android/dialer/speeddial/SpeedDialFragment.java
@@ -130,7 +130,6 @@ public class SpeedDialFragment extends Fragment {
             contextMenuBackground,
             new SpeedDialContextMenuItemListener(
                 getActivity(),
-                getChildFragmentManager(),
                 new UpdateSpeedDialAdapterListener(),
                 speedDialLoaderListener),
             layoutManager);
@@ -321,19 +320,17 @@ public class SpeedDialFragment extends Fragment {
       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 @@ public class SpeedDialFragment extends Fragment {
                 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 @@ public class SpeedDialFragment extends Fragment {
         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 c5a3ea3ed..325af238a 100644
--- a/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java
+++ b/java/com/android/dialer/speeddial/loader/SpeedDialUiItem.java
@@ -162,61 +162,77 @@ public abstract class SpeedDialUiItem {
   }
 
   /**
-   * 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 @@ public abstract class SpeedDialUiItem {
    * 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();