Fixes crash in Assistant.
Root cause of the crash was committing a query without first having
pended a query.
This can occur in a notifications flow or a device actions flow in
which an interaction did not originate from the UI.
We rely on pending/committing the query in the interaction model to
clear the previous query from the UI. The fix, in this case, was just
to pend an empty query so that we have something to commit.
Bug: b:111923411
Change-Id: I53848f856375cf220538738634e7543a455c0dcc
Reviewed-on: https://chromium-review.googlesource.com/1155153
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#579271}diff --git a/ash/assistant/assistant_interaction_controller.cc b/ash/assistant/assistant_interaction_controller.cc
index 1e7a19c..107ea9b 100644
--- a/ash/assistant/assistant_interaction_controller.cc
+++ b/ash/assistant/assistant_interaction_controller.cc
@@ -155,7 +155,18 @@
assistant_interaction_model_.SetInputModality(InputModality::kVoice);
assistant_interaction_model_.SetMicState(MicState::kOpen);
} else {
- // In the case of a non-voice interaction, we commit the pending query.
+ // TODO(b/112000321): It should not be possible to reach this code without
+ // having previously pended a query. It does currently happen, however, in
+ // the case of notifications and device action queries which bypass the
+ // AssistantInteractionController when beginning an interaction. To address
+ // this, we temporarily pend an empty text query to commit until we can do
+ // development to expose something more meaningful.
+ if (assistant_interaction_model_.pending_query().type() ==
+ AssistantQueryType::kEmpty) {
+ assistant_interaction_model_.SetPendingQuery(
+ std::make_unique<AssistantTextQuery>());
+ }
+
assistant_interaction_model_.CommitPendingQuery();
assistant_interaction_model_.SetMicState(MicState::kClosed);
diff --git a/ash/assistant/model/assistant_interaction_model.cc b/ash/assistant/model/assistant_interaction_model.cc
index f110de4..87ebc2b 100644
--- a/ash/assistant/model/assistant_interaction_model.cc
+++ b/ash/assistant/model/assistant_interaction_model.cc
@@ -76,6 +76,8 @@
}
void AssistantInteractionModel::CommitPendingQuery() {
+ DCHECK_NE(pending_query_->type(), AssistantQueryType::kEmpty);
+
committed_query_ = std::move(pending_query_);
pending_query_ = std::make_unique<AssistantEmptyQuery>();
diff --git a/ash/assistant/model/assistant_query.h b/ash/assistant/model/assistant_query.h
index 8efa1adb..08f76c2a7 100644
--- a/ash/assistant/model/assistant_query.h
+++ b/ash/assistant/model/assistant_query.h
@@ -13,6 +13,7 @@
// AssistantQueryType ----------------------------------------------------------
+// TODO(dmblack): Rename kEmpty to kNull.
// Defines possible types of an Assistant query.
enum class AssistantQueryType {
kEmpty, // See AssistantEmptyQuery.
@@ -44,6 +45,7 @@
// AssistantEmptyQuery ---------------------------------------------------------
+// TODO(dmblack): Rename to AssistantNullQuery.
// An empty Assistant query used to signify the absence of an Assistant query.
class AssistantEmptyQuery : public AssistantQuery {
public:
@@ -63,7 +65,7 @@
// An Assistant text query.
class AssistantTextQuery : public AssistantQuery {
public:
- explicit AssistantTextQuery(const std::string& text)
+ explicit AssistantTextQuery(const std::string& text = std::string())
: AssistantQuery(AssistantQueryType::kText), text_(text) {}
~AssistantTextQuery() override = default;
diff --git a/ash/assistant/ui/main_stage/assistant_main_stage.cc b/ash/assistant/ui/main_stage/assistant_main_stage.cc
index 080f726c..6634b00 100644
--- a/ash/assistant/ui/main_stage/assistant_main_stage.cc
+++ b/ash/assistant/ui/main_stage/assistant_main_stage.cc
@@ -119,16 +119,6 @@
PreferredSizeChanged();
}
-void AssistantMainStage::OnViewIsDeleting(views::View* view) {
- if (view == committed_query_view_) {
- committed_query_view_ = nullptr;
- UpdateCommittedQueryViewSpacer();
- } else if (view == pending_query_view_) {
- pending_query_view_ = nullptr;
- UpdateSuggestionContainer();
- }
-}
-
void AssistantMainStage::InitLayout(AssistantController* assistant_controller) {
SetLayoutManager(std::make_unique<views::FillLayout>());
@@ -209,7 +199,11 @@
return;
query_layout_container_->RemoveChildView(committed_query_view_);
+
delete committed_query_view_;
+ committed_query_view_ = nullptr;
+
+ UpdateCommittedQueryViewSpacer();
}
void AssistantMainStage::OnPendingQueryChanged(const AssistantQuery& query) {
@@ -232,13 +226,12 @@
void AssistantMainStage::OnPendingQueryCleared() {
if (pending_query_view_) {
query_layout_container_->RemoveChildView(pending_query_view_);
+
delete pending_query_view_;
- } else {
- // We only need to update the suggestion container when we are not deleting
- // the pending query view. Deleting the pending query view will trigger an
- // update on the suggestion container itself.
- UpdateSuggestionContainer();
+ pending_query_view_ = nullptr;
}
+
+ UpdateSuggestionContainer();
}
void AssistantMainStage::UpdateCommittedQueryViewSpacer() {
diff --git a/ash/assistant/ui/main_stage/assistant_main_stage.h b/ash/assistant/ui/main_stage/assistant_main_stage.h
index 9d861c5..7be74700 100644
--- a/ash/assistant/ui/main_stage/assistant_main_stage.h
+++ b/ash/assistant/ui/main_stage/assistant_main_stage.h
@@ -35,7 +35,6 @@
void OnViewBoundsChanged(views::View* view) override;
void OnViewPreferredSizeChanged(views::View* view) override;
void OnViewVisibilityChanged(views::View* view) override;
- void OnViewIsDeleting(views::View* view) override;
// AssistantInteractionModelObserver:
void OnCommittedQueryChanged(const AssistantQuery& query) override;
diff --git a/ash/assistant/ui/main_stage/assistant_query_view.cc b/ash/assistant/ui/main_stage/assistant_query_view.cc
index 25802b27..306a75c 100644
--- a/ash/assistant/ui/main_stage/assistant_query_view.cc
+++ b/ash/assistant/ui/main_stage/assistant_query_view.cc
@@ -88,7 +88,7 @@
break;
}
case AssistantQueryType::kEmpty:
- label_->SetText(base::string16());
+ SetText(std::string());
break;
}
}
@@ -123,7 +123,6 @@
label_->SizeToFit(width());
PreferredSizeChanged();
- SetVisible(true);
}
} // namespace ash
