Commit 0bd94b50 authored by Vladislav Kaznacheev's avatar Vladislav Kaznacheev Committed by Commit Bot

Exit metalayer on failed gesture if invoked from stylus button

This should minimize disruption when a user is writing/sketching
while accidentally holding down the stylus button.

Bug: 767441
Test: ash_unittests --gtest_filter=PaletteTrayTest*
Change-Id: I155d6c7b0997dd56680aaae8a005fba074a15865
Reviewed-on: https://chromium-review.googlesource.com/688486
Commit-Queue: Vladislav Kaznacheev <kaznacheev@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Reviewed-by: default avatarJacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504843}
parent d1c5cff3
......@@ -168,6 +168,8 @@ void HighlighterController::RecognizeGesture() {
base::Unretained(this)));
recognized_gesture_counter_++;
} else if (observer_) {
observer_->HandleFailedSelection();
}
gesture_counter_++;
......
......@@ -61,6 +61,17 @@ const FastInkPoints& HighlighterControllerTestApi::predicted_points() const {
void HighlighterControllerTestApi::HandleSelection(const gfx::Rect& rect) {
handle_selection_called_ = true;
selection_ = rect;
// This is mimicking the logic implemented PaletteDelegateChromeOS,
// which should eventually move to HighlighterController (crbug/761120).
CallMetalayerDone();
}
void HighlighterControllerTestApi::HandleFailedSelection() {
handle_failed_selection_called_ = true;
// This is mimicking the logic implemented PaletteDelegateChromeOS,
// which should eventually move to HighlighterController (crbug/761120).
if (via_button_)
CallMetalayerDone();
}
} // namespace ash
......@@ -6,6 +6,7 @@
#define ASH_HIGHLIGHTER_HIGHLIGHTER_CONTROLLER_TEST_API_H_
#include "ash/highlighter/highlighter_selection_observer.h"
#include "base/callback.h"
#include "base/macros.h"
#include "ui/gfx/geometry/rect.h"
......@@ -22,6 +23,14 @@ class HighlighterControllerTestApi : public HighlighterSelectionObserver {
explicit HighlighterControllerTestApi(HighlighterController* instance);
~HighlighterControllerTestApi() override;
void SetMetalayerDone(base::OnceClosure done) {
metalayer_done_ = std::move(done);
}
void CallMetalayerDone() {
if (!metalayer_done_.is_null())
std::move(metalayer_done_).Run();
}
void SetViaButton(bool via_button) { via_button_ = via_button; }
void SetEnabled(bool enabled);
void DestroyPointerView();
void SimulateInterruptedStrokeTimeout();
......@@ -32,19 +41,30 @@ class HighlighterControllerTestApi : public HighlighterSelectionObserver {
const FastInkPoints& points() const;
const FastInkPoints& predicted_points() const;
void ResetSelection() { handle_selection_called_ = false; }
void ResetSelection() {
handle_selection_called_ = false;
handle_failed_selection_called_ = false;
}
bool handle_selection_called() const { return handle_selection_called_; }
bool handle_failed_selection_called() const {
return handle_failed_selection_called_;
}
const gfx::Rect& selection() const { return selection_; }
private:
// HighlighterSelectionObserver:
void HandleSelection(const gfx::Rect& rect) override;
void HandleFailedSelection() override;
HighlighterController* instance_;
bool handle_selection_called_ = false;
bool handle_failed_selection_called_ = false;
gfx::Rect selection_;
base::OnceClosure metalayer_done_;
bool via_button_ = false;
DISALLOW_COPY_AND_ASSIGN(HighlighterControllerTestApi);
};
......
......@@ -168,6 +168,7 @@ TEST_F(HighlighterControllerTest, HighlighterGestures) {
GetEventGenerator().MoveTouch(gfx::Point(200, 200));
GetEventGenerator().ReleaseTouch();
EXPECT_FALSE(controller_test_api_->handle_selection_called());
EXPECT_TRUE(controller_test_api_->handle_failed_selection_called());
// An almost horizontal stroke is recognized
controller_test_api_->ResetSelection();
......@@ -176,6 +177,7 @@ TEST_F(HighlighterControllerTest, HighlighterGestures) {
GetEventGenerator().MoveTouch(gfx::Point(300, 102));
GetEventGenerator().ReleaseTouch();
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
// Horizontal stroke selection rectangle should:
// have the same horizontal center line as the stroke bounding box,
......@@ -192,6 +194,7 @@ TEST_F(HighlighterControllerTest, HighlighterGestures) {
GetEventGenerator().MoveTouch(gfx::Point(100, 100));
GetEventGenerator().ReleaseTouch();
EXPECT_FALSE(controller_test_api_->handle_selection_called());
EXPECT_TRUE(controller_test_api_->handle_failed_selection_called());
// An almost closed G-like shape is recognized
controller_test_api_->ResetSelection();
......@@ -203,6 +206,7 @@ TEST_F(HighlighterControllerTest, HighlighterGestures) {
GetEventGenerator().MoveTouch(gfx::Point(200, 20));
GetEventGenerator().ReleaseTouch();
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("0,0 200x100", controller_test_api_->selection().ToString());
// A closed diamond shape is recognized
......@@ -215,6 +219,7 @@ TEST_F(HighlighterControllerTest, HighlighterGestures) {
GetEventGenerator().MoveTouch(gfx::Point(100, 50));
GetEventGenerator().ReleaseTouch();
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("0,50 200x200", controller_test_api_->selection().ToString());
}
......@@ -246,6 +251,7 @@ TEST_F(HighlighterControllerTest, HighlighterGesturesScaled) {
controller_test_api_->ResetSelection();
TraceRect(original_rect);
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
const gfx::Rect selection = controller_test_api_->selection();
EXPECT_TRUE(inflated.Contains(selection));
......@@ -266,6 +272,7 @@ TEST_F(HighlighterControllerTest, HighlighterGesturesRotated) {
controller_test_api_->ResetSelection();
TraceRect(trace);
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("200,100 400x300", controller_test_api_->selection().ToString());
// Rotate to 90 degrees
......@@ -273,6 +280,7 @@ TEST_F(HighlighterControllerTest, HighlighterGesturesRotated) {
controller_test_api_->ResetSelection();
TraceRect(trace);
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("100,900 300x400", controller_test_api_->selection().ToString());
// Rotate to 180 degrees
......@@ -280,6 +288,7 @@ TEST_F(HighlighterControllerTest, HighlighterGesturesRotated) {
controller_test_api_->ResetSelection();
TraceRect(trace);
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("900,600 400x300", controller_test_api_->selection().ToString());
// Rotate to 270 degrees
......@@ -287,6 +296,7 @@ TEST_F(HighlighterControllerTest, HighlighterGesturesRotated) {
controller_test_api_->ResetSelection();
TraceRect(trace);
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("600,200 300x400", controller_test_api_->selection().ToString());
}
......@@ -307,6 +317,7 @@ TEST_F(HighlighterControllerTest, InterruptedStroke) {
GetEventGenerator().ReleaseTouch();
EXPECT_TRUE(controller_test_api_->IsWaitingToResumeStroke());
EXPECT_FALSE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_FALSE(controller_test_api_->IsFadingAway());
GetEventGenerator().MoveTouch(gfx::Point(0, 200));
......@@ -315,6 +326,7 @@ TEST_F(HighlighterControllerTest, InterruptedStroke) {
GetEventGenerator().ReleaseTouch();
EXPECT_FALSE(controller_test_api_->IsWaitingToResumeStroke());
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_EQ("0,100 300x100", controller_test_api_->selection().ToString());
// Repeat the same gesture, but simulate a timeout after the gap. This should
......@@ -326,11 +338,13 @@ TEST_F(HighlighterControllerTest, InterruptedStroke) {
GetEventGenerator().ReleaseTouch();
EXPECT_TRUE(controller_test_api_->IsWaitingToResumeStroke());
EXPECT_FALSE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_FALSE(controller_test_api_->IsFadingAway());
controller_test_api_->SimulateInterruptedStrokeTimeout();
EXPECT_FALSE(controller_test_api_->IsWaitingToResumeStroke());
EXPECT_TRUE(controller_test_api_->handle_selection_called());
EXPECT_FALSE(controller_test_api_->handle_failed_selection_called());
EXPECT_TRUE(controller_test_api_->IsFadingAway());
}
......
......@@ -17,6 +17,7 @@ class HighlighterSelectionObserver {
virtual ~HighlighterSelectionObserver() {}
virtual void HandleSelection(const gfx::Rect& rect) = 0;
virtual void HandleFailedSelection() = 0;
};
} // namespace ash
......
......@@ -56,8 +56,9 @@ class PaletteDelegate {
virtual void CancelPartialScreenshot() = 0;
// Shows the metalayer. |done| is called when the metalayer session has
// finished.
virtual void ShowMetalayer(base::OnceClosure done) = 0;
// finished. |via_button| is true if metalayer is invoked via a hardware
// stylus button.
virtual void ShowMetalayer(base::OnceClosure done, bool via_button) = 0;
// Hides the metalayer.
virtual void HideMetalayer() = 0;
......
......@@ -50,7 +50,7 @@ class PaletteDelegateImpl : public PaletteDelegate {
done.Run();
}
void CancelPartialScreenshot() override {}
void ShowMetalayer(base::OnceClosure done) override {}
void ShowMetalayer(base::OnceClosure done, bool via_button) override {}
void HideMetalayer() override {}
private:
......
......@@ -291,11 +291,10 @@ class PaletteTrayTestWithVoiceInteraction : public PaletteTrayTest {
expected_on_press);
}
void DestroyPointerView() { highlighter_test_api_->DestroyPointerView(); }
std::unique_ptr<HighlighterControllerTestApi> highlighter_test_api_;
private:
std::unique_ptr<HighlighterController> highlighter_controller_;
std::unique_ptr<HighlighterControllerTestApi> highlighter_test_api_;
// Owned by |ui|.
base::SimpleTestTickClock* simulated_clock_ = nullptr;
......@@ -334,13 +333,33 @@ TEST_F(PaletteTrayTestWithVoiceInteraction, MetalayerToolActivatesHighlighter) {
EXPECT_FALSE(highlighter_showing());
// Press/drag over a regular (non-palette) location. This should activate the
// highlighter.
// highlighter. Note that a diagonal stroke does not create a valid selection.
highlighter_test_api_->ResetSelection();
DragAndAssertMetalayer("tool enabled", origin, ui::EF_NONE,
true /* enables metalayer */,
true /* metalayer stays enabled after the press */,
true /* highlighter shown on press */);
// When metalayer is entered normally (not via stylus button), a failed
// selection should not exit the mode.
// NOTE that this is not testing the real logic in PaletteDelegateChromeOS,
// but the logic in HighlighterControllerTestApi (which is mimicking
// PaletteDelegateChromeOS). Once PaletteDelegateChromeOS is refactored
// (crbug/761120) the assertions below become more useful.
EXPECT_TRUE(highlighter_test_api_->handle_failed_selection_called());
EXPECT_TRUE(metalayer_enabled());
// A successfull selection should exit the metalayer mode.
SCOPED_TRACE("horizontal stroke");
highlighter_test_api_->ResetSelection();
generator.MoveTouch(gfx::Point(100, 100));
generator.PressTouch();
EXPECT_TRUE(metalayer_enabled());
generator.MoveTouch(gfx::Point(300, 100));
generator.ReleaseTouch();
EXPECT_TRUE(highlighter_test_api_->handle_selection_called());
EXPECT_FALSE(metalayer_enabled());
SCOPED_TRACE("drag over palette");
DestroyPointerView();
highlighter_test_api_->DestroyPointerView();
// Press/drag over the palette button. This should not activate the
// highlighter, but should disable the palette tool instead.
gfx::Point palette_point = palette_tray_->GetBoundsInScreen().CenterPoint();
......@@ -439,6 +458,8 @@ TEST_F(PaletteTrayTestWithVoiceInteraction,
WaitDragAndAssertMetalayer("with button", origin, ui::EF_LEFT_MOUSE_BUTTON,
true /* enables metalayer */,
false /* no highlighter on press */);
// Metalayer mode entered via the stylus button should not be sticky.
EXPECT_FALSE(metalayer_enabled());
// Repeat the previous step without a pause, make sure that the palette tool
// is not toggled, and the highlighter is enabled immediately.
......@@ -452,7 +473,7 @@ TEST_F(PaletteTrayTestWithVoiceInteraction,
true /* enables metalayer */, true /* highlighter shown on press */);
// The barrel button should not work on the lock screen.
DestroyPointerView();
highlighter_test_api_->DestroyPointerView();
GetSessionControllerClient()->RequestLockScreen();
EXPECT_FALSE(test_api_->GetPaletteToolManager()->IsToolActive(
PaletteToolId::METALAYER));
......@@ -472,7 +493,7 @@ TEST_F(PaletteTrayTestWithVoiceInteraction,
EXPECT_FALSE(test_api_->GetPaletteToolManager()->IsToolActive(
PaletteToolId::METALAYER));
DestroyPointerView();
highlighter_test_api_->DestroyPointerView();
EXPECT_FALSE(highlighter_showing());
DragAndAssertMetalayer("disabled", origin, ui::EF_LEFT_MOUSE_BUTTON,
false /* no metalayer */,
......
......@@ -47,11 +47,14 @@ void TestPaletteDelegate::TakePartialScreenshot(const base::Closure& done) {
void TestPaletteDelegate::CancelPartialScreenshot() {}
void TestPaletteDelegate::ShowMetalayer(base::OnceClosure done) {
metalayer_done_ = std::move(done);
void TestPaletteDelegate::ShowMetalayer(base::OnceClosure done,
bool via_button) {
++show_metalayer_count_;
if (highlighter_test_api_)
if (highlighter_test_api_) {
highlighter_test_api_->SetMetalayerDone(std::move(done));
highlighter_test_api_->SetViaButton(via_button);
highlighter_test_api_->SetEnabled(true);
}
}
void TestPaletteDelegate::HideMetalayer() {
......
......@@ -46,8 +46,6 @@ class TestPaletteDelegate : public PaletteDelegate {
int hide_metalayer_count() const { return hide_metalayer_count_; }
void CallMetalayerDone() { std::move(metalayer_done_).Run(); }
void set_highlighter_test_api(HighlighterControllerTestApi* api) {
highlighter_test_api_ = api;
}
......@@ -63,7 +61,7 @@ class TestPaletteDelegate : public PaletteDelegate {
void TakeScreenshot() override;
void TakePartialScreenshot(const base::Closure& done) override;
void CancelPartialScreenshot() override;
void ShowMetalayer(base::OnceClosure done) override;
void ShowMetalayer(base::OnceClosure done, bool via_button) override;
void HideMetalayer() override;
int create_note_count_ = 0;
......@@ -76,7 +74,6 @@ class TestPaletteDelegate : public PaletteDelegate {
bool should_show_palette_ = false;
int show_metalayer_count_ = 0;
int hide_metalayer_count_ = 0;
base::OnceClosure metalayer_done_;
HighlighterControllerTestApi* highlighter_test_api_ = nullptr;
......
......@@ -56,8 +56,10 @@ PaletteToolId MetalayerMode::GetToolId() const {
void MetalayerMode::OnEnable() {
CommonPaletteTool::OnEnable();
Shell::Get()->palette_delegate()->ShowMetalayer(base::BindOnce(
&MetalayerMode::OnMetalayerSessionComplete, weak_factory_.GetWeakPtr()));
Shell::Get()->palette_delegate()->ShowMetalayer(
base::BindOnce(&MetalayerMode::OnMetalayerSessionComplete,
weak_factory_.GetWeakPtr()),
activated_via_button_);
delegate()->HidePalette();
}
......@@ -65,6 +67,7 @@ void MetalayerMode::OnDisable() {
CommonPaletteTool::OnDisable();
Shell::Get()->palette_delegate()->HideMetalayer();
activated_via_button_ = false;
}
const gfx::VectorIcon& MetalayerMode::GetActiveTrayIcon() const {
......@@ -130,6 +133,7 @@ void MetalayerMode::OnTouchEvent(ui::TouchEvent* event) {
delegate()->RecordPaletteOptionsUsage(
PaletteToolIdToPaletteTrayOptions(GetToolId()),
PaletteInvocationMethod::SHORTCUT);
activated_via_button_ = true;
delegate()->EnableTool(GetToolId());
}
event->StopPropagation();
......
......@@ -85,6 +85,9 @@ class ASH_EXPORT MetalayerMode : public CommonPaletteTool,
base::TimeTicks previous_stroke_end_;
// True when the mode is activated via the stylus barrel button.
bool activated_via_button_ = false;
base::WeakPtrFactory<MetalayerMode> weak_factory_;
DISALLOW_COPY_AND_ASSIGN(MetalayerMode);
......
......@@ -2,6 +2,8 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "ash/highlighter/highlighter_controller.h"
#include "ash/highlighter/highlighter_controller_test_api.h"
#include "ash/shell.h"
#include "ash/shell_test_api.h"
#include "ash/system/palette/mock_palette_tool_delegate.h"
......@@ -34,11 +36,19 @@ class MetalayerToolTest : public AshTestBase {
palette_tool_delegate_ = base::MakeUnique<MockPaletteToolDelegate>();
tool_ = base::MakeUnique<MetalayerMode>(palette_tool_delegate_.get());
highlighter_controller_ = base::MakeUnique<HighlighterController>();
highlighter_test_api_ = base::MakeUnique<HighlighterControllerTestApi>(
highlighter_controller_.get());
test_palette_delegate()->set_highlighter_test_api(
highlighter_test_api_.get());
}
void TearDown() override {
// This needs to be called first to remove the event handler before the
// shell instance gets torn down.
test_palette_delegate()->set_highlighter_test_api(nullptr);
highlighter_test_api_.reset();
highlighter_controller_.reset();
tool_.reset();
AshTestBase::TearDown();
}
......@@ -48,6 +58,8 @@ class MetalayerToolTest : public AshTestBase {
}
protected:
std::unique_ptr<HighlighterController> highlighter_controller_;
std::unique_ptr<HighlighterControllerTestApi> highlighter_test_api_;
std::unique_ptr<MockPaletteToolDelegate> palette_tool_delegate_;
std::unique_ptr<PaletteTool> tool_;
......@@ -162,7 +174,7 @@ TEST_F(MetalayerToolTest, MetalayerCallbackDisablesPaletteTool) {
// Calling the associated callback |metalayer_done| will disable the tool.
EXPECT_CALL(*palette_tool_delegate_.get(),
DisableTool(PaletteToolId::METALAYER));
test_palette_delegate()->CallMetalayerDone();
highlighter_test_api_->CallMetalayerDone();
}
} // namespace ash
......@@ -50,6 +50,10 @@ class VoiceInteractionSelectionObserver
delay_timer_.reset();
}
void set_disable_on_failed_selection(bool disable_on_failed_selection) {
disable_on_failed_selection_ = disable_on_failed_selection;
}
bool start_session_pending() const { return delay_timer_.get(); }
private:
......@@ -63,7 +67,15 @@ class VoiceInteractionSelectionObserver
base::Unretained(this), rect),
false /* not repeating */);
delay_timer_->Reset();
DisableMetalayer();
}
void HandleFailedSelection() override {
if (disable_on_failed_selection_)
DisableMetalayer();
}
void DisableMetalayer() {
DCHECK(on_selection_done_);
// This will disable the metalayer tool, which will result in a synchronous
// call to PaletteDelegateChromeOS::HideMetalayer.
......@@ -83,6 +95,7 @@ class VoiceInteractionSelectionObserver
std::unique_ptr<base::Timer> delay_timer_;
base::OnceClosure on_selection_done_;
bool disable_on_failed_selection_ = true;
DISALLOW_COPY_AND_ASSIGN(VoiceInteractionSelectionObserver);
};
......@@ -219,7 +232,8 @@ void PaletteDelegateChromeOS::CancelPartialScreenshot() {
ash::Shell::Get()->screenshot_controller()->CancelScreenshotSession();
}
void PaletteDelegateChromeOS::ShowMetalayer(base::OnceClosure done) {
void PaletteDelegateChromeOS::ShowMetalayer(base::OnceClosure done,
bool via_button) {
auto* service =
arc::ArcVoiceInteractionFrameworkService::GetForBrowserContext(profile_);
if (!service)
......@@ -231,6 +245,7 @@ void PaletteDelegateChromeOS::ShowMetalayer(base::OnceClosure done) {
base::MakeUnique<VoiceInteractionSelectionObserver>(profile_);
}
highlighter_selection_observer_->set_on_selection_done(std::move(done));
highlighter_selection_observer_->set_disable_on_failed_selection(via_button);
ash::Shell::Get()->highlighter_controller()->SetEnabled(true);
}
......
......@@ -43,7 +43,7 @@ class PaletteDelegateChromeOS
void TakeScreenshot() override;
void TakePartialScreenshot(const base::Closure& done) override;
void CancelPartialScreenshot() override;
void ShowMetalayer(base::OnceClosure done) override;
void ShowMetalayer(base::OnceClosure done, bool via_button) override;
void HideMetalayer() override;
// user_manager::UserManager::UserSessionStateObserver:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment