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 6 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
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(
Copy link
Contributor

@dkwingsmt dkwingsmt Aug 21, 2021

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?

Copy link
Contributor Author

@justinmc justinmc Aug 23, 2021

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
47 changes: 47 additions & 0 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 @@ -22,6 +22,7 @@ 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";
Expand Down Expand Up @@ -103,6 +104,52 @@ TEST(PlatformHandler, RejectsGettingUnknownTypes) {
}));
}

TEST(PlatformHandler, HasStringsCallsThrough) {
Copy link
Contributor Author

@justinmc justinmc Aug 5, 2021

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

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Aug 5, 2021

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.

Copy link
Contributor Author

@justinmc justinmc Aug 5, 2021

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);

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>(kHasStringsClipboardMethod,
std::move(args)));

// 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,
auto key) { result->NotImplemented(); });

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

TEST(PlatformHandler, RejectsHasStringsOnUnknownTypes) {
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>(kHasStringsClipboardMethod,
std::move(args)));

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) {
JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope(
reply, reply_size, &result);
}));
}

TEST(PlatformHandler, SettingTextCallsThrough) {
TestBinaryMessenger messenger;
TestPlatformHandler platform_handler(&messenger);
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