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

Conversation

@moko256
Copy link
Contributor

@moko256 moko256 commented Aug 13, 2021

This PR will able to merge after:

This PR implements clipboard in platform handler in UWP.
I tested it by these steps:

  • Selecting TextField in flutter uwp app.
  • Typing some characters.
  • Performing right click and checking Paste behavior in the context menu.
Clipboard content Paste behavior
Text (copied from another app) appeared, performing paste by clicking
Image (from Print Screen key) disappeared

Fixes one item in flutter/flutter#70214.

No changes in flutter/tests.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@google-cla google-cla bot added the cla: yes label Aug 13, 2021
@moko256 moko256 force-pushed the moko256_uwp_clipboard branch from 68250c8 to 2423079 Compare August 27, 2021 04:07
@moko256 moko256 marked this pull request as ready for review August 27, 2021 04:48
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I attempted a review but I am not familiar with this component. Perhaps @cbracken @stuartmorgan

// Waiting `DataPackageView::GetTextAsync()` using `TResult.get()` on the
// platform thread will causes the application stop, so we continue
// response on this callback.
content.GetTextAsync().Completed([result = std::move(result), key](
Copy link
Member

Choose a reason for hiding this comment

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

Not super familiar with these APIs, but, does this have an error handler as well that we may need to account for? Perhaps one that also gets invoked when the window is not in the foreground.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before here I check window is in foreground, so I think this should throw no errors. I created an example and checked app doesn't crash from the engine. (I will post it on the comments.)

@moko256
Copy link
Contributor Author

moko256 commented Sep 10, 2021

Reading clipboard every 1 sec to check no engine crash from background.

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Clipboard Loop',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Clipboard Loop'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({Key? key, required this.title}) : super(key: key);
  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  _MyHomePageState() {
    startLoopingCheckClipboard().then((_) {});
  }

  Future<void> startLoopingCheckClipboard() async {
    while (true) {
      await Future.delayed(const Duration(seconds: 1));
      try {
        var text = (await Clipboard.getData("text/plain"))?.text;
        if (text != null) {
          _updateContentAndError(text, null);
        } else {
          _updateContentAndError(null, "clipboard is null");
        }
      } catch (e) {
        _updateContentAndError(null, e.toString());
      }
    }
  }

  String? _content;
  String? _error;

  void _updateContentAndError(String? content, String? error) {
    setState(() {
      _content = content;
      _error = error;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'Clipboard content',
            ),
            Text(
              _content ?? "",
              style: Theme.of(context).textTheme.headline4,
            ),
            Text(
              _error ?? "",
              style: Theme.of(context)
                  .textTheme
                  .headline4
                  ?.copyWith(color: Theme.of(context).errorColor),
            ),
          ],
        ),
      ),
    );
  }
}

result->Success(rapidjson::Document());
}
} else {
result->Success(rapidjson::Document());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the example, I found that Clipboard.getData() throws PlatformException when app is not focused, in web, but here Clipboard.getData() send success. I will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chinmaygarde
Copy link
Member

cc @clarkezone. I am not sure about these APIs but they the implementation looks good to me modulo the error callback.

@clarkezone
Copy link

I'm blocked currently due to inability to build the UWP target on my machine.

}
} else {
result->Error(kClipboardError,
"Unable to read clipboard. The window is not focused.");
Copy link
Member

@cbracken cbracken Sep 27, 2021

Choose a reason for hiding this comment

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

This conditional block is pretty huge; consider inverting the condition and writing this as a bailout-style check where if the window isn't in the foreground, you set the error and return. Similar comment for the inner case where the clipboard contains no text; that would also put the comment closer to the result->Success(empty document) handling of that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM other than the style nit above.

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 27, 2021
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 27, 2021
@cbracken cbracken force-pushed the moko256_uwp_clipboard branch from dbce0be to 4ac26a2 Compare September 27, 2021 22:19
@cbracken
Copy link
Member

cbracken commented Sep 27, 2021

The build_and_test_linux_unopt_debug failure was caused by a recent change in our cirrus API token. I've rebased to tip-of-tree to pick up the new API key introduced in #28584.

@cbracken cbracken added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 27, 2021
@cbracken cbracken merged commit c94ee5b into flutter:master Sep 28, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2021
dnfield pushed a commit to dnfield/engine that referenced this pull request Oct 6, 2021
This PR implements clipboard in platform handler in UWP.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes needs tests 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.

5 participants