Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

hasStrings on Windows #27749

Merged
fluttergithubbot merged 20 commits into from
Aug 25, 2021
Merged

hasStrings on Windows #27749

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
70a0f5e
Working hasStrings on windows
justinmc Aug 4, 2021
c9d633a
Analyzer
justinmc Aug 4, 2021
39990f3
Blind attempt at tests since I'm unable to run them right now
justinmc Aug 4, 2021
c3060cd
Analyzer fix
justinmc Aug 5, 2021
83af633
Fix windows newlines
justinmc Aug 5, 2021
b7c562e
Analyzer fix
justinmc Aug 5, 2021
64c5751
Add unittests file to build, it must have been missed accidentally
justinmc Aug 5, 2021
85528fd
Tests compiling but not passsing
justinmc Aug 9, 2021
a640fcc
Analyzer fixes
justinmc Aug 9, 2021
aa4dc9e
Merge branch 'master' into has-strings-windows
justinmc Aug 20, 2021
61e2fa5
Old tests pass thanks to work in fix-platform-handler-unittests branch
justinmc Aug 20, 2021
5a35ed2
First new test passes too
justinmc Aug 20, 2021
6cef1f2
All tests pass
justinmc Aug 20, 2021
1c6744a
Attempt at formatting
justinmc Aug 20, 2021
e4587a8
Analyzer fixes
justinmc Aug 20, 2021
4c5afb3
Fix my poor string conversions
justinmc Aug 21, 2021
b81e8f3
Rename HasStrings to GetHasStrings
justinmc Aug 23, 2021
3a161be
Merge branch 'master' into has-strings-windows
justinmc Aug 24, 2021
620e7aa
Use original MethodCall format with correct type
justinmc Aug 24, 2021
b06f2c8
Remove accidental printf!
justinmc Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions shell/platform/windows/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ executable("flutter_windows_unittests") {
"keyboard_key_embedder_handler_unittests.cc",
"keyboard_key_handler_unittests.cc",
"keyboard_unittests.cc",
"platform_handler_unittests.cc",
"testing/flutter_window_win32_test.cc",
"testing/flutter_window_win32_test.h",
"testing/mock_window_binding_handler.cc",
Expand Down
10 changes: 10 additions & 0 deletions shell/platform/windows/platform_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
static constexpr char kChannelName[] = "flutter/platform";

static constexpr char kGetClipboardDataMethod[] = "Clipboard.getData";
static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings";
static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData";

static constexpr char kTextPlainFormat[] = "text/plain";
Expand Down Expand Up @@ -46,6 +47,15 @@ void PlatformHandler::HandleMethodCall(
return;
}
GetPlainText(std::move(result), kTextKey);
} else if (method.compare(kHasStringsClipboardMethod) == 0) {
// Only one string argument is expected.
const rapidjson::Value& format = method_call.arguments()[0];

if (strcmp(format.GetString(), kTextPlainFormat) != 0) {
result->Error(kClipboardError, kUnknownClipboardFormatMessage);
return;
}
HasStrings(std::move(result));
} else if (method.compare(kSetClipboardDataMethod) == 0) {
const rapidjson::Value& document = *method_call.arguments();
rapidjson::Value::ConstMemberIterator itr = document.FindMember(kTextKey);
Expand Down
5 changes: 5 additions & 0 deletions shell/platform/windows/platform_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ class PlatformHandler {
std::unique_ptr<MethodResult<rapidjson::Document>> result,
std::string_view key) = 0;

// Provides a boolean to |result| as the value in a dictionary at key
// "value" representing whether or not the clipboard has a non-empty string.
virtual void HasStrings(

@dkwingsmt dkwingsmt Aug 21, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm revisiting an established design. But this name looks pretty confusing. Why is it HasString instead of HasString? And can we rename it to a "verb", such as GetHasString to represent the fact that it is a query from the framework?

@justinmc justinmc Aug 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plural HasStrings came from native iOS hasStrings, which is where all of this started. I'll change the Windows methods to GetHasStrings while keeping the method channel name the same "Clipboard.hasStrings".

std::unique_ptr<MethodResult<rapidjson::Document>> result) = 0;

// Sets the clipboard's plain text to |text|, and reports the result (either
// an error, or null for success) to |result|.
virtual void SetPlainText(
Expand Down
78 changes: 60 additions & 18 deletions shell/platform/windows/platform_handler_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

#include <memory>

#include "flutter/shell/platform/common/client_wrapper/include/flutter/binary_messenger.h"

@justinmc justinmc Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was causing a compiler error for some reason, removing it seemed to work 🤷

#include "flutter/shell/platform/common/json_method_codec.h"
#include "flutter/shell/platform/windows/testing/test_binary_messenger.h"
#include "gmock/gmock.h"
Expand All @@ -22,9 +21,11 @@ using ::testing::_;
static constexpr char kChannelName[] = "flutter/platform";

static constexpr char kGetClipboardDataMethod[] = "Clipboard.getData";
static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings";
static constexpr char kSetClipboardDataMethod[] = "Clipboard.setData";

static constexpr char kTextPlainFormat[] = "text/plain";
static constexpr char kFakeContentType[] = "text/madeupcontenttype";

@justinmc justinmc Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fake content type must begin with "text" even though it's supposed to fail. If it doesn't, then rapidjson can't parse the json at all.


// Test implementation of PlatformHandler to allow testing the PlatformHandler
// logic.
Expand All @@ -38,7 +39,9 @@ class TestPlatformHandler : public PlatformHandler {
// |PlatformHandler|
MOCK_METHOD2(GetPlainText,
void(std::unique_ptr<MethodResult<rapidjson::Document>>,
const char*));
std::string_view key));
MOCK_METHOD1(HasStrings,
void(std::unique_ptr<MethodResult<rapidjson::Document>>));
MOCK_METHOD2(SetPlainText,
void(const std::string&,
std::unique_ptr<MethodResult<rapidjson::Document>>));
Expand All @@ -61,12 +64,10 @@ TEST(PlatformHandler, GettingTextCallsThrough) {
TestBinaryMessenger messenger;
TestPlatformHandler platform_handler(&messenger);

auto args = std::make_unique<rapidjson::Document>(rapidjson::kArrayType);
auto& allocator = args->GetAllocator();
args->PushBack(kTextPlainFormat, allocator);
auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall(
MethodCall<rapidjson::Document>(kGetClipboardDataMethod,
std::move(args)));
std::ostringstream jsonStringStream;
jsonStringStream << "{\"method\":\"" << kGetClipboardDataMethod
<< "\",\"args\":\"" << kTextPlainFormat << "\"}";
std::string jsonString = jsonStringStream.str();

// Set up a handler to call a response on |result| so that it doesn't log
// on destruction about leaking.
Expand All @@ -77,27 +78,68 @@ TEST(PlatformHandler, GettingTextCallsThrough) {

EXPECT_CALL(platform_handler, GetPlainText(_, ::testing::StrEq("text")));
EXPECT_TRUE(messenger.SimulateEngineMessage(
kChannelName, encoded->data(), encoded->size(),
[](const uint8_t* reply, size_t reply_size) {}));
kChannelName, reinterpret_cast<const unsigned char*>(jsonString.c_str()),
jsonString.size(), [](const uint8_t* reply, size_t reply_size) {}));
}

TEST(PlatformHandler, RejectsGettingUnknownTypes) {
TestBinaryMessenger messenger;
TestPlatformHandler platform_handler(&messenger);

auto args = std::make_unique<rapidjson::Document>(rapidjson::kArrayType);
auto& allocator = args->GetAllocator();
args->PushBack("madeup/contenttype", allocator);
auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall(
MethodCall<rapidjson::Document>(kGetClipboardDataMethod,
std::move(args)));
std::ostringstream jsonStringStream;
jsonStringStream << "{\"method\":\"" << kGetClipboardDataMethod
<< "\",\"args\":\"" << kFakeContentType << "\"}";
std::string jsonString = jsonStringStream.str();

MockMethodResult result;
// Requsting an unknow content type is an error.
EXPECT_CALL(result, ErrorInternal(_, _, _));
EXPECT_TRUE(messenger.SimulateEngineMessage(
kChannelName, encoded->data(), encoded->size(),
[&](const uint8_t* reply, size_t reply_size) {
kChannelName, reinterpret_cast<const unsigned char*>(jsonString.c_str()),
jsonString.size(), [&](const uint8_t* reply, size_t reply_size) {
JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope(
reply, reply_size, &result);
}));
}

TEST(PlatformHandler, HasStringsCallsThrough) {

@justinmc justinmc Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was NOT able to run these tests locally! Are we ok if the checks just pass here on Github? Which unittests.exe file in out/ is supposed to run the tests in this file? @dkwingsmt

@stuartmorgan-g stuartmorgan-g Aug 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be flutter_windows_unittests but it looks like it was never actually added to the build. That would be good to fix in this PR.

Are we ok if the checks just pass here on Github?

In theory, but since it's missing from the build the GitHub checks aren't telling us anything.

@justinmc justinmc Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I felt like something was up. Thanks, I'll add it!

TestBinaryMessenger messenger;
TestPlatformHandler platform_handler(&messenger);

std::ostringstream jsonStringStream;
jsonStringStream << "{\"method\":\"" << kHasStringsClipboardMethod
<< "\",\"args\":\"" << kTextPlainFormat << "\"}";
std::string jsonString = jsonStringStream.str();

// Set up a handler to call a response on |result| so that it doesn't log
// on destruction about leaking.
ON_CALL(platform_handler, HasStrings)
.WillByDefault(
[](std::unique_ptr<MethodResult<rapidjson::Document>> result) {
result->NotImplemented();
});

EXPECT_CALL(platform_handler, HasStrings(_));
EXPECT_TRUE(messenger.SimulateEngineMessage(
kChannelName, reinterpret_cast<const unsigned char*>(jsonString.c_str()),
jsonString.size(), [](const uint8_t* reply, size_t reply_size) {}));
}

TEST(PlatformHandler, RejectsHasStringsOnUnknownTypes) {
TestBinaryMessenger messenger;
TestPlatformHandler platform_handler(&messenger);

std::ostringstream jsonStringStream;
jsonStringStream << "{\"method\":\"" << kHasStringsClipboardMethod
<< "\",\"args\":\"" << kFakeContentType << "\"}";
std::string jsonString = jsonStringStream.str();

MockMethodResult result;
// Requsting an unknow content type is an error.
EXPECT_CALL(result, ErrorInternal(_, _, _));
EXPECT_TRUE(messenger.SimulateEngineMessage(
kChannelName, reinterpret_cast<const unsigned char*>(jsonString.c_str()),
jsonString.size(), [&](const uint8_t* reply, size_t reply_size) {
JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope(
reply, reply_size, &result);
}));
Expand Down
20 changes: 20 additions & 0 deletions shell/platform/windows/platform_handler_win32.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "flutter/shell/platform/windows/flutter_windows_view.h"
#include "flutter/shell/platform/windows/string_conversion.h"

static constexpr char kValueKey[] = "value";

namespace flutter {

namespace {
Expand Down Expand Up @@ -229,6 +231,24 @@ void PlatformHandlerWin32::GetPlainText(
result->Success(document);
}

void PlatformHandlerWin32::HasStrings(
std::unique_ptr<MethodResult<rapidjson::Document>> result) {
ScopedClipboard clipboard;
if (!clipboard.Open(std::get<HWND>(*view_->GetRenderTarget()))) {
rapidjson::Document error_code;
error_code.SetInt(::GetLastError());
result->Error(kClipboardError, "Unable to open clipboard", error_code);
return;
}

rapidjson::Document document;
document.SetObject();
rapidjson::Document::AllocatorType& allocator = document.GetAllocator();
document.AddMember(rapidjson::Value(kValueKey, allocator),
rapidjson::Value(clipboard.HasString()), allocator);
result->Success(document);
}

void PlatformHandlerWin32::SetPlainText(
const std::string& text,
std::unique_ptr<MethodResult<rapidjson::Document>> result) {
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/windows/platform_handler_win32.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ class PlatformHandlerWin32 : public PlatformHandler {
void GetPlainText(std::unique_ptr<MethodResult<rapidjson::Document>> result,
std::string_view key) override;

// |PlatformHandler|
void HasStrings(
std::unique_ptr<MethodResult<rapidjson::Document>> result) override;

// |PlatformHandler|
void SetPlainText(
const std::string& text,
Expand Down
6 changes: 6 additions & 0 deletions shell/platform/windows/platform_handler_winuwp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ void PlatformHandlerWinUwp::GetPlainText(
result->NotImplemented();
}

void PlatformHandlerWinUwp::HasStrings(
std::unique_ptr<MethodResult<rapidjson::Document>> result) {
// TODO: Implement. See https://github.com/flutter/flutter/issues/70214.
result->NotImplemented();
}

void PlatformHandlerWinUwp::SetPlainText(
const std::string& text,
std::unique_ptr<MethodResult<rapidjson::Document>> result) {
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/windows/platform_handler_winuwp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ class PlatformHandlerWinUwp : public PlatformHandler {
void GetPlainText(std::unique_ptr<MethodResult<rapidjson::Document>> result,
std::string_view key) override;

// |PlatformHandler|
void HasStrings(
std::unique_ptr<MethodResult<rapidjson::Document>> result) override;

// |PlatformHandler|
void SetPlainText(
const std::string& text,
Expand Down