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

Conversation

@justinmc
Copy link
Contributor

@justinmc justinmc commented Jul 27, 2021

Implements the hasStrings method for the Windows platform. This allows the framework to check if the clipboard has content without accessing the content itself on platforms that show a notification to the user when the clipboard is read. Here on Windows I don't believe it shows a notification anyway, but it's good to not read the clipboard when we don't have to.

Related Issues

Main issue: flutter/flutter#60145

hasStrings iOS: #19859
hasStrings Android: #20393
hasStrings Mac: #20531
hasStrings Linux: #21388

hasStrings framework: flutter/flutter#61774, flutter/flutter#87678

@justinmc justinmc self-assigned this Jul 27, 2021
@google-cla google-cla bot added the cla: yes label Jul 27, 2021
@justinmc justinmc force-pushed the has-strings-windows branch from 6606f5f to c9d633a Compare August 4, 2021 18:24
@justinmc justinmc force-pushed the has-strings-windows branch from 6703606 to 83af633 Compare August 5, 2021 04:54
@justinmc justinmc force-pushed the has-strings-windows branch from e9a558a to b7c562e Compare August 5, 2021 05:38
@justinmc justinmc marked this pull request as ready for review August 5, 2021 16:10
}));
}

TEST(PlatformHandler, HasStringsCallsThrough) {
Copy link
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

Copy link
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.

Copy link
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!

@justinmc justinmc changed the title (WIP) hasStrings on Windows hasStrings on Windows Aug 5, 2021
@justinmc
Copy link
Contributor Author

justinmc commented Aug 5, 2021

Let me know if I've missed anyone else that I should get to review this.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

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

LGTM. I don't see any problems here, although I'm wondering if we should consider moving this API to embedder.h.

@stuartmorgan-g
Copy link
Contributor

I'm wondering if we should consider moving this API to embedder.h.

AFAIK the plan of record is still to wrap all of the method channels in embedder APIs, it's just a matter of actually finding the time to do it.

@justinmc justinmc force-pushed the has-strings-windows branch from 6f69f5c to 85528fd Compare August 9, 2021 20:09
@justinmc
Copy link
Contributor Author

justinmc commented Aug 9, 2021

I've gotten the tests to compile but they don't pass.

Test output .\out\host_debug_unopt\flutter_windows_unittests.exe

[INFO:test_timeout_listener.cc(76)] Test timeout of 30 seconds per test case will be enforced. [==========] Running 77 tests from 15 test suites. [----------] Global test environment set-up. [----------] 6 tests from StringConversion [ RUN ] StringConversion.Utf16FromUtf8Empty [ OK ] StringConversion.Utf16FromUtf8Empty (0 ms) [ RUN ] StringConversion.Utf16FromUtf8Ascii [ OK ] StringConversion.Utf16FromUtf8Ascii (0 ms) [ RUN ] StringConversion.Utf16FromUtf8Unicode [ OK ] StringConversion.Utf16FromUtf8Unicode (0 ms) [ RUN ] StringConversion.Utf8FromUtf16Empty [ OK ] StringConversion.Utf8FromUtf16Empty (0 ms) [ RUN ] StringConversion.Utf8FromUtf16Ascii [ OK ] StringConversion.Utf8FromUtf16Ascii (0 ms) [ RUN ] StringConversion.Utf8FromUtf16Unicode [ OK ] StringConversion.Utf8FromUtf16Unicode (0 ms) [----------] 6 tests from StringConversion (4 ms total) [----------] 1 test from PlatformHandler [ RUN ] PlatformHandler.GettingTextCallsThrough Assertion failed: IsString(), file ../../third_party/rapidjson/include\rapidjson/document.h, line 1773

@chinmaygarde
Copy link
Member

@justinmc Are you blocked on this?

@justinmc
Copy link
Contributor Author

@chinmaygarde Yes, I'm struggling to get those old tests to pass now that they've been added to the build. I've pinged @clarkezone for advice.

@moko256 moko256 mentioned this pull request Aug 13, 2021
10 tasks
@cbracken cbracken removed the request for review from stuartmorgan-g August 16, 2021 21:28
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
@justinmc justinmc deleted the has-strings-windows branch August 25, 2021 03:00
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2021
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 25, 2021
* f575a5d Roll Dart SDK from abecc4bc4521 to 72f462ed2d34 (6 revisions) (flutter/engine#28279)

* 25262ad Roll Skia from 0f36d11c1dcf to 6f90c702e6c8 (4 revisions) (flutter/engine#28280)

* 6c8ef61 Correct the return value of the method RunInIsolateScope (flutter/engine#28161)

* 2126214 Roll Skia from 6f90c702e6c8 to 877858a19502 (2 revisions) (flutter/engine#28282)

* 62934d0 Eliminate unnecessary null check in fixture (flutter/engine#28281)

* e218f5f Roll Skia from 877858a19502 to e43714f490ec (6 revisions) (flutter/engine#28283)

* b17c2a6 Roll Dart SDK from 72f462ed2d34 to 35c81c55d91f (1 revision) (flutter/engine#28284)

* bba6439 Roll Skia from e43714f490ec to 7b3edfa65923 (1 revision) (flutter/engine#28285)

* 902b16d Roll buildroot to 14b15b2 (flutter/engine#28286)

* 687affe hasStrings on Windows (flutter/engine#27749)

* 33342cb Roll Clang Mac from T2LfgbJBs... to XvHmkqWWH... (flutter/engine#28288)

* 5508373 Roll Clang Linux from cIFYQ49sp... to dO8igrHyL... (flutter/engine#28289)

* c2e289e Roll Skia from 7b3edfa65923 to 80fc31bc61a9 (2 revisions) (flutter/engine#28290)

* 1867ca4 Roll Dart SDK from 35c81c55d91f to 4c4bf41da092 (1 revision) (flutter/engine#28291)

* d75f033 Roll Skia from 80fc31bc61a9 to 9155b338bb9c (1 revision) (flutter/engine#28292)

* 9a2383c Roll Dart SDK from 4c4bf41da092 to 0507607e1380 (1 revision) (flutter/engine#28294)

* 16a94e6 Roll Skia from 9155b338bb9c to 02c7dac0be64 (1 revision) (flutter/engine#28295)

* 64891d0 Roll Skia from 02c7dac0be64 to 0305e86ee48c (1 revision) (flutter/engine#28296)

* 58b1b60 Roll Dart SDK from 0507607e1380 to 2a693f47e303 (1 revision) (flutter/engine#28297)

* 9c78284 Roll Skia from 0305e86ee48c to 1e16937f4734 (2 revisions) (flutter/engine#28300)

* 63f83fc Run the Android Robolectric tests using Gradle (flutter/engine#28084)

* 93f6188 Roll Dart SDK from 2a693f47e303 to 6fecad447467 (1 revision) (flutter/engine#28301)

* d6677ba Roll Skia from 1e16937f4734 to d34406abf959 (1 revision) (flutter/engine#28303)

* 6243b46 Roll Skia from d34406abf959 to b61014d31064 (5 revisions) (flutter/engine#28305)

* eaa9f8b Revert "Roll Dart SDK from 2a693f47e303 to 6fecad447467 (1 revision) (#28301)" (flutter/engine#28309)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants