Fix bugs in the display notification

ash/system/chromeos/screen_layout_observer.cc

Index: ash/system/chromeos/screen_layout_observer.cc
diff --git a/ash/system/chromeos/screen_layout_observer.cc b/ash/system/chromeos/screen_layout_observer.cc
index a804840a608cd2997d88037dd5b4147f31a05400..602c4af53e7b8fce9763c8524566fa4a745396d9 100644
--- a/ash/system/chromeos/screen_layout_observer.cc
+++ b/ash/system/chromeos/screen_layout_observer.cc
@@ -102,35 +102,29 @@ void OpenSettingsFromNotification() {
   }
 }
 
-// Returns the name of the currently connected external display. This should not
-// be used when the external display is used for mirroring.
-base::string16 GetExternalDisplayName() {
+// Returns the name of the currently connected external display whose ID is
+// |external_display_id|. This should not be used when the external display is
+// used for mirroring.
+base::string16 GetExternalDisplayName(int64_t external_display_id) {
+  DCHECK(!display::Display::IsInternalDisplayId(external_display_id));
+
   display::DisplayManager* display_manager = GetDisplayManager();
   DCHECK(!display_manager->IsInMirrorMode());
 
-  int64_t external_id = display::kInvalidDisplayId;
-  for (size_t i = 0; i < display_manager->GetNumDisplays(); ++i) {
-    int64_t id = display_manager->GetDisplayAt(i).id();
-    if (!display::Display::IsInternalDisplayId(id)) {
-      external_id = id;
-      break;
-    }
-  }
-
-  if (external_id == display::kInvalidDisplayId)
+  if (external_display_id == display::kInvalidDisplayId)
     return l10n_util::GetStringUTF16(IDS_DISPLAY_NAME_UNKNOWN);
 
   // The external display name may have an annotation of "(width x height)" in
   // case that the display is rotated or its resolution is changed.
-  base::string16 name = GetDisplayName(external_id);
+  base::string16 name = GetDisplayName(external_display_id);
   const display::ManagedDisplayInfo& display_info =
-      display_manager->GetDisplayInfo(external_id);
+      display_manager->GetDisplayInfo(external_display_id);
   if (display_info.GetActiveRotation() != display::Display::ROTATE_0 ||
       display_info.configured_ui_scale() != 1.0f ||
       !display_info.overscan_insets_in_dip().IsEmpty()) {
     name =
         l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_ANNOTATED_NAME,
-                                   name, GetDisplaySize(external_id));
+                                   name, GetDisplaySize(external_display_id));
   } else if (display_info.overscan_insets_in_dip().IsEmpty() &&
              display_info.has_overscan()) {
     name = l10n_util::GetStringFUTF16(
@@ -142,41 +136,121 @@ base::string16 GetExternalDisplayName() {
   return name;
 }
 
-base::string16 GetDisplayMessage(base::string16* additional_message_out) {
+// Returns true if docked mode is currently enabled.
+bool IsDockedModeEnabled() {
   display::DisplayManager* display_manager = GetDisplayManager();
-  if (display_manager->GetNumDisplays() > 1) {
-    if (display::Display::HasInternalDisplay()) {
-      return l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED,
-                                        GetExternalDisplayName());
+  if (display::Display::HasInternalDisplay()) {
+    // We have an internal display but it's not one of the active displays.
+    for (size_t i = 0; i < display_manager->GetNumDisplays(); ++i) {
+      if (display::Display::IsInternalDisplayId(
+              display_manager->GetDisplayAt(i).id())) {
+        return false;
+      }
+
+      return true;
     }
-    return l10n_util::GetStringUTF16(
-        IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED_NO_INTERNAL);
   }
 
-  if (display_manager->IsInMirrorMode()) {
+  return false;
+}
+
+// Returns the notification message that should be shown to the user when the
+// docked mode is entered.
+base::string16 GetDockedModeEnabledMessage(
+    base::string16* out_additional_message) {
+  DCHECK(IsDockedModeEnabled());
+
+  if (out_additional_message) {

[oshima, 2017/01/24 17:49:41]

can this be null?

[afakhry, 2017/01/24 22:13:22]

No. Replaced with a DCHECK

+    *out_additional_message = ash::SubstituteChromeOSDeviceType(
+        IDS_ASH_STATUS_TRAY_DISPLAY_DOCKED_DESCRIPTION);
+  }
+
+  return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_DOCKED);
+}
+
+// Fills |out_message| with the appropriate notification message the should be
+// shown to the user when the status of the mirror mode is changed, and return
+// true. Otherwise, returns false and |out_message| is not modified.
+bool GetMirrorModeChangedMessage(bool current_mirror_mode,
+                                 base::string16* out_message,
+                                 base::string16* out_additional_message) {

[oshima, 2017/01/24 17:49:41]

It's better to separate to

GetExit/EnterMirrorModeMessage

and let caller decide.

[afakhry, 2017/01/24 22:13:22]

Done.

+  display::DisplayManager* display_manager = GetDisplayManager();
+  if (current_mirror_mode) {
     if (display::Display::HasInternalDisplay()) {
-      return l10n_util::GetStringFUTF16(
+      *out_message = l10n_util::GetStringFUTF16(
           IDS_ASH_STATUS_TRAY_DISPLAY_MIRRORING,
           GetDisplayName(display_manager->mirroring_display_id()));
+    } else {
+      *out_message = l10n_util::GetStringUTF16(
+          IDS_ASH_STATUS_TRAY_DISPLAY_MIRRORING_NO_INTERNAL);
     }
-    return l10n_util::GetStringUTF16(
-        IDS_ASH_STATUS_TRAY_DISPLAY_MIRRORING_NO_INTERNAL);
+
+    return true;
   }
 
-  if (display_manager->IsInUnifiedMode())
-    return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_UNIFIED);
+  // Handle disabling mirror mode as a result of going to docked mode when we
+  // only have a single display (this means we actually have two physical
+  // displays, one of which is the internal display, but they were in mirror
+  // mode, and hence considered as one. Closing the internal display disables
+  // mirror mode and we still have a single active display).
+  // Also make sure we're not in unified mode as this is also considered as
+  // a single display.
+  if (IsDockedModeEnabled() && display_manager->GetNumDisplays() == 1 &&
+      !display_manager->IsInUnifiedMode()) {
+    *out_message = GetDockedModeEnabledMessage(out_additional_message);
+    return true;
+  }
 
-  int64_t primary_id = display::Screen::GetScreen()->GetPrimaryDisplay().id();
-  if (display::Display::HasInternalDisplay() &&
-      !(display::Display::IsInternalDisplayId(primary_id))) {
-    if (additional_message_out) {
-      *additional_message_out = ash::SubstituteChromeOSDeviceType(
-          IDS_ASH_STATUS_TRAY_DISPLAY_DOCKED_DESCRIPTION);
-    }
-    return l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_DOCKED);
+  if (display_manager->GetNumDisplays() > 2) {
+    // Mirror mode was turned off due to having more than two displays. Show
+    // a message that mirror mode for 3+ displays is not supported.
+    *out_message =
+        l10n_util::GetStringUTF16(IDS_ASH_DISPLAY_MIRRORING_NOT_SUPPORTED);
+    return true;
   }
 
-  return base::string16();
+  // Mirror mode was turned off; other messages should be shown e.g. extended
+  // mode is on, ... etc.
+  return false;
+}
+
+// Returns the notification message that should be shown to the user when there
+// is a change in the unified mode status.
+base::string16 GetUnifiedModeChangedMessage(bool current_unified_mode) {
+  return l10n_util::GetStringUTF16(
+      current_unified_mode ? IDS_ASH_STATUS_TRAY_DISPLAY_UNIFIED
+                           : IDS_ASH_STATUS_TRAY_DISPLAY_UNIFIED_EXITING);
+}
+
+base::string16 GetDisplayRemovedMessage(
+    const display::ManagedDisplayInfo& removed_display_info,
+    base::string16* out_additional_message) {
+  // Removing the internal display means entering docked mode.
+  if (display::Display::IsInternalDisplayId(removed_display_info.id()))
+    return GetDockedModeEnabledMessage(out_additional_message);
+
+  return l10n_util::GetStringFUTF16(
+      IDS_ASH_STATUS_TRAY_DISPLAY_REMOVED,
+      base::UTF8ToUTF16(removed_display_info.name()));
+}
+
+base::string16 GetDisplayAddedMessage(int64_t added_display_id,
+                                      base::string16* additional_message_out) {
+  if (!display::Display::HasInternalDisplay()) {
+    return l10n_util::GetStringUTF16(
+        IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED_NO_INTERNAL);
+  }
+
+  if (display::Display::IsInternalDisplayId(added_display_id)) {
+    // Adding the internal display means exiting docked mode (IFF we are not
+    // exiting unified mode. This case should have already been handled by the
+    // time this function is called).
+    return l10n_util::GetStringUTF16(
+        IDS_ASH_STATUS_TRAY_DISPLAY_DOCKED_EXITING);
+  }
+
+  return l10n_util::GetStringFUTF16(IDS_ASH_STATUS_TRAY_DISPLAY_EXTENDED,
+                                    GetExternalDisplayName(added_display_id));
 }
 
 }  // namespace
@@ -210,35 +284,62 @@ bool ScreenLayoutObserver::GetDisplayMessageForNotification(
     const ScreenLayoutObserver::DisplayInfoMap& old_info,
     base::string16* message_out,
     base::string16* additional_message_out) {
-  // Display is added or removed. Use the same message as the one in
-  // the system tray.
-  if (display_info_.size() != old_info.size()) {
-    *message_out = GetDisplayMessage(additional_message_out);
+  // Detect changes in the mirror mode status.
+  if (old_is_in_mirrored_mode_ != current_is_in_mirrored_mode_ &&
+      GetMirrorModeChangedMessage(current_is_in_mirrored_mode_, message_out,
+                                  additional_message_out)) {
     return true;
   }
 
-  for (DisplayInfoMap::const_iterator iter = display_info_.begin();
-       iter != display_info_.end(); ++iter) {
-    DisplayInfoMap::const_iterator old_iter = old_info.find(iter->first);
-    // The display's number is same but different displays. This happens
-    // for the transition between docked mode and mirrored display. Falls back
-    // to GetDisplayMessage().
-    if (old_iter == old_info.end()) {
-      *message_out = GetDisplayMessage(additional_message_out);
+  // Detect changes in the unified mode.
+  if (old_is_in_unified_mode_ != current_is_in_unified_mode_) {
+    *message_out = GetUnifiedModeChangedMessage(current_is_in_unified_mode_);
+    return true;
+  }
+
+  // Displays are added or removed.
+  if (display_info_.size() < old_info.size()) {
+    // A display has been removed.
+    for (const auto& iter : old_info) {
+      if (display_info_.count(iter.first))
+        continue;
+
+      *message_out =
+          GetDisplayRemovedMessage(iter.second, additional_message_out);
+      return true;
+    }
+  } else if (display_info_.size() > old_info.size()) {
+    // A display has been added.
+    for (const auto& iter : display_info_) {
+      if (old_info.count(iter.first))
+        continue;
+
+      *message_out = GetDisplayAddedMessage(iter.first, additional_message_out);
       return true;
     }
+  }
 
-    if (iter->second.configured_ui_scale() !=
+  for (const auto& iter : display_info_) {
+    DisplayInfoMap::const_iterator old_iter = old_info.find(iter.first);
+    if (old_iter == old_info.end()) {
+      // The display's number is same but different displays. This happens
+      // for the transition between docked mode and mirrored display.
+      // This condition can never be reached here, since it is handled above.
+      NOTREACHED();
+      return false;
+    }
+
+    if (iter.second.configured_ui_scale() !=
         old_iter->second.configured_ui_scale()) {
       *additional_message_out = l10n_util::GetStringFUTF16(
           IDS_ASH_STATUS_TRAY_DISPLAY_RESOLUTION_CHANGED,
-          GetDisplayName(iter->first), GetDisplaySize(iter->first));
+          GetDisplayName(iter.first), GetDisplaySize(iter.first));
       return true;
     }
-    if (iter->second.GetActiveRotation() !=
+    if (iter.second.GetActiveRotation() !=
         old_iter->second.GetActiveRotation()) {
       int rotation_text_id = 0;
-      switch (iter->second.GetActiveRotation()) {
+      switch (iter.second.GetActiveRotation()) {
         case display::Display::ROTATE_0:
           rotation_text_id = IDS_ASH_STATUS_TRAY_DISPLAY_STANDARD_ORIENTATION;
           break;
@@ -253,7 +354,7 @@ bool ScreenLayoutObserver::GetDisplayMessageForNotification(
           break;
       }
       *additional_message_out = l10n_util::GetStringFUTF16(
-          IDS_ASH_STATUS_TRAY_DISPLAY_ROTATED, GetDisplayName(iter->first),
+          IDS_ASH_STATUS_TRAY_DISPLAY_ROTATED, GetDisplayName(iter.first),
           l10n_util::GetStringUTF16(rotation_text_id));
       return true;
     }
@@ -304,6 +405,12 @@ void ScreenLayoutObserver::OnDisplayConfigurationChanged() {
   DisplayInfoMap old_info;
   UpdateDisplayInfo(&old_info);
 
+  old_is_in_mirrored_mode_ = current_is_in_mirrored_mode_;
+  current_is_in_mirrored_mode_ = GetDisplayManager()->IsInMirrorMode();
+
+  old_is_in_unified_mode_ = current_is_in_unified_mode_;
+  current_is_in_unified_mode_ = GetDisplayManager()->IsInUnifiedMode();
+
   if (!show_notifications_for_testing)
     return;