From 16b77d4d484dca3d4a070709cfb046c6fba8c993 Mon Sep 17 00:00:00 2001 From: calderwoodra Date: Fri, 27 Apr 2018 23:12:57 -0700 Subject: Updated logic on showing voice/video/sms options in favorites menus. Bug: 78492066 Test: numerous PiperOrigin-RevId: 194635336 Change-Id: I7be0efad4dc9e11beceb02c9b2f4c719d29dbbd1 --- java/com/android/dialer/speeddial/ContextMenu.java | 44 +++------ .../dialer/speeddial/SpeedDialFragment.java | 53 +---------- .../dialer/speeddial/loader/SpeedDialUiItem.java | 101 ++++++++++++++------- 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. - * - *

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,67 +395,23 @@ 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> speedDialLoaderListener; private final UpdateSpeedDialAdapterListener updateAdapterListener; SpeedDialContextMenuItemListener( FragmentActivity activity, - FragmentManager childFragmentManager, UpdateSpeedDialAdapterListener updateAdapterListener, SupportUiListener> 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) { 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: + * + *

*/ @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}. * + *

These channels have a few very strictly enforced assumption that are used heavily throughout + * the codebase. Those assumption are that: + * + *

    + *
  1. 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}. + *
  2. 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. + *
+ * + * 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 channels(); -- cgit v1.2.3