Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clipboard] getData implementation added #65

Closed

Conversation

pkosko
Copy link

@pkosko pkosko commented Apr 15, 2021

[Before] Clipboard.getData feature was not implemented

[After] Dart code below is able to gather system clipboard data:

      Future<ClipboardData?> d = Clipboard.getData(Clipboard.kTextPlain);
      void dataCallback(ClipboardData? d) {
        if (d != null) {
          String? text = d.text;
          if(text != null) {
            print("Clipboard data $text");
          }
        }
      }
      d.then(dataCallback);

[TODO] Need to consider adding some mechanism of synchronization of access to native cbhm API
to provide proper behaviour in case of multiple calls

Change related to PlatformChannel Clipboard.getData() part of https://github.sec.samsung.net/f-project/home/issues/35
https://github.sec.samsung.net/f-project/home/issues/35

Necessary dependency was added to tizen_tools with PR flutter-tizen/tizen_tools#8

@pkosko pkosko requested review from a team and removed request for a team April 15, 2021 10:09
pkosko added a commit to pkosko/tizen_tools that referenced this pull request Apr 15, 2021
@pkosko pkosko requested a review from a team April 15, 2021 10:16
const rapidjson::Value& format = call.arguments()[0];

// https://api.flutter.dev/flutter/services/Clipboard/kTextPlain-constant.html
// API supports only kTextPlain format, hovewer cbhm API supports also other formats

Choose a reason for hiding this comment

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

Suggested change
// API supports only kTextPlain format, hovewer cbhm API supports also other formats
// API supports only kTextPlain format, however cbhm API supports also other formats


ret = cbhm_selection_get(cbhm_handle, selection_type, cbhm_selection_data_cb, data);
if (CBHM_ERROR_NONE != ret) {
result->Error(kUnknownClipboardError, "Failed to gather data.");

Choose a reason for hiding this comment

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

Shouldn't we free data in case of failure?

Choose a reason for hiding this comment

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

Also result is invalidated in line 89.

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I will fix it!

@swift-kim

This comment has been minimized.

@swift-kim
Copy link
Member

Sorry. I was not aware of the tizen_tools PR!

[Before] Clipboard.getData feature was not implemented

[After] Dart code below is able to gather system clipboard data:
      Future<ClipboardData?> d = Clipboard.getData(Clipboard.kTextPlain);
      void dataCallback(ClipboardData? d) {
        if (d != null) {
          String? text = d.text;
          if(text != null) {
            print("Clipboard data $text");
          }
        }
      }
      d.then(dataCallback);

[TODO] Need to consider adding some mechanism of synchronization of access to native cbhm API
to provide proper behaviour in case of multiple calls
@pkosko pkosko force-pushed the clipboard_getData branch from 6079c1c to 0435387 Compare April 15, 2021 12:24
int ret = cbhm_open_service (&cbhm_handle);
if (CBHM_ERROR_NONE != ret) {
result->Error(kUnknownClipboardError, "Failed to initialize cbhm service.");
return;

Choose a reason for hiding this comment

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

Shouldn't is_processing be set to false before return?

@@ -37,7 +49,99 @@ void PlatformChannel::HandleMethodCall(
} else if (method == "HapticFeedback.vibrate") {
result->NotImplemented();
} else if (method == "Clipboard.getData") {
result->NotImplemented();
const rapidjson::Value& format = call.arguments()[0];

Choose a reason for hiding this comment

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

Consider moving the body of this "if" to a separate method to make PlatformChannel::HandleMethodCall shorter.

return;
}

struct method_data_t {

Choose a reason for hiding this comment

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

Shouldn't we use Google coding style here and name this MethodData?

@swift-kim
Copy link
Member

swift-kim commented Apr 16, 2021

Tip: If you're using VS Code you can easily format your code by applying the following.

  • Enable "Format on Save" in the settings
  • Install the "C/C++" extension
  • Add "C_Cpp.clang_format_fallbackStyle": "Google" to the workspace settings

Caution: Make sure a correct clang-format executable is being used by VS Code. To verify, run

$ which clang-format

in the command line and check the location of the executable. (depot_tools should not appear in the path.)

The extension documentation says:

The full path of the clang-format executable. If not specified, and clang-format is available in the environment path, that is used. If not found in the environment path, a copy of clang-format bundled with the extension will be used.

To specify the clang-format executable path for VS Code, first install clang-format-11 (sudo apt install clang-format-11) and add this line to the workspace settings:

"C_Cpp.clang_format_path": "/usr/bin/clang-format-11"

@swift-kim
Copy link
Member

Thank you for your contribution!

As far as I can see the CBHM API is mobile-only, so probably the embedder with this change will not even load on non-mobile devices (e.g. watch) because libcbhm will not exist on those devices. I'm currently thinking about how to release different embedders for different profiles (mobile/wearable/tv/iot) and integrate with the flutter-tizen tool. The design is not fixed yet but it will use compiler switches like #ifdef TV_PROFILE. You will have to wait a while until it is implemented for your change to be merged. Otherwise you can suggest any other ways to deal with the problem!

pkosko added a commit to pkosko/tizen_tools that referenced this pull request Apr 19, 2021
pkosko added a commit to pkosko/tizen_tools that referenced this pull request Apr 19, 2021
@swift-kim
Copy link
Member

Now #67 has been merged and you can guard any code that is for mobile only using #ifdef MOBILE_PROFILE or #if defined(MOBILE_PROFILE). Also you will have to change BUILD.gn as follows:

diff --git a/shell/platform/tizen/BUILD.gn b/shell/platform/tizen/BUILD.gn
index 1ea9bcf30..bb9837748 100644
--- a/shell/platform/tizen/BUILD.gn
+++ b/shell/platform/tizen/BUILD.gn
@@ -38,6 +38,7 @@ config("tizen_rootstrap_include_dirs") {
     "$custom_sysroot/usr/include",
     "$custom_sysroot/usr/include/appfw",
     "$custom_sysroot/usr/include/base",
+    "$custom_sysroot/usr/include/cbhm",
     "$custom_sysroot/usr/include/dlog",
     "$custom_sysroot/usr/include/ecore-1",
     "$custom_sysroot/usr/include/ecore-evas-1",
@@ -141,6 +142,10 @@ template("embedder_for_profile") {
       libs += [ "ecore_wl2" ]
     }

+    if (target_name == "mobile") {
+      libs += [ "cbhm" ]
+    }
+
     cflags_cc = [
       "-Wno-newline-eof",
       "-Wno-macro-redefined",

@pkosko
Copy link
Author

pkosko commented Apr 27, 2021

Unfortunately, implementation using public CBHM API has issue which disqualifies it as stable copy/paste support even on mobile only. In case of Flutter application using simple TextField in its UI:

TextField(
              decoration: InputDecoration(
                border: OutlineInputBorder(),
                hintText: 'Enter a search term',
              ),
            ),

, framework automatically calls Clipboard.getData() method during first run of application. According to my analysis and testing, in such case native API (cbhm_selection_get) does not trigger success callback (cbhm_selection_data_cb), nor return any error. Because current implementation is based on mutexes to protect accessing from different threads, this scenario causes a deadlock-like API state which always return error "Already processing in other thread" (because is_processing flag was not released in callback). I've tried to workaround this problem, but my all ideas were very ugly hacks which also does not guarantee memory safety and proper API behaviours in all cases.

Because of above issue of native API, I think that it is better to not include this PR to final code.

@pkosko pkosko closed this Apr 27, 2021
@pkosko
Copy link
Author

pkosko commented Apr 27, 2021

I've prepared much simpler implementation #76 which uses naive in-application clipboard. Please share your opinion.

@swift-kim swift-kim mentioned this pull request Jun 26, 2021
12 tasks
swift-kim pushed a commit that referenced this pull request Sep 1, 2022
…rt API. Minor refactor of tessellator to supply fill type to method rather than to object. (#65)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants