WhoAteDaCake commented 2 years ago

Changes proposed in this pull request:

Add support for linux clipboard.

@lc-soft

IssueHunt Summary

### Referenced issues This pull request has been submitted to: - [#216: Add clipboard support](https://issuehunt.io/repos/5293802/issues/216) ---

commit-lint[bot] commented 2 years ago

Chore

git: Remove LCUI/include from gitignore, so new headers could be added (ac01ab8)
clang formatting (9903e01)
value name changes (af60ddb)
cleanup implementation by reducing header files (70ac671)
implementation cleanup (c4f4265)
function name change (0d1dd46)

Features

clipboard: initial layout setup (2913d97)
clipboard: headers for UseClipboard access (094d985)
clipboard: further callback execution (c36780b)
clipboard: retrieve text at widget level (a0302f1)
textedit: insert paste event text (a47f1b9)
textedit: do not insert keys if control char is held. (c3b3c96)
clipboard: add more notes and error messages for implementation (494f3f6)
clipboard: function definitions required for copying (cbf2b69)
clipboard: further steps for implementing paste to clipboard (0128138)
clipboard: further steps for implementing copy (6a373f3)
clipboard: add copy support by copying full widget text (f42c4a4)
clipboard: pass down length together with wchar (a16dda2)
clipboard: better struct wrapper (7f199c0)
clipboard: make clipboard more generic (2a58b59)

Bug Fixes

clipboard: remove static + cleanup initialisation (dcd35fb)
clipboard: remove static from UseClipboard (1daf973)
keyboard: do not trigger keyboard event twice (1e2f777)
keyboard: undo keyboard changes and add note to look into it more (63026fa)
clipboard: remove switch statement, since we already bind for specific event (26b7f67)
clipboard: do not use key, since text enum is occupied (b6bf68c)
keyboard: do not submit keypress if ctrl is held. (20af98d)

Contributors

WhoAteDaCake

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR: - `@Commit-Lint merge patch` will merge dependabot PR on "patch" versions (X.X.Y - Y change) - `@Commit-Lint merge minor` will merge dependabot PR on "minor" versions (X.Y.Y - Y change) - `@Commit-Lint merge major` will merge dependabot PR on "major" versions (Y.Y.Y - Y change) - `@Commit-Lint merge disable` will desactivate merge dependabot PR - `@Commit-Lint review` will approve dependabot PR - `@Commit-Lint stop review` will stop approve dependabot PR

codecov[bot] commented 2 years ago

Codecov Report

Merging #271 (d5685e9) into v3.0-dev (b3a0a94) will increase coverage by 0.41%. The diff coverage is 24.76%.

@@             Coverage Diff              @@
##           v3.0-dev     #271      +/-   ##
============================================
+ Coverage     55.07%   55.48%   +0.41%     
============================================
  Files            80       82       +2     
  Lines         14890    15097     +207     
============================================
+ Hits           8200     8377     +177     
- Misses         6690     6720      +30

Impacted Files	Coverage Δ
lib/platform/src/linux/linux_x11clipboard.c	`0.00% <0.00%> (ø)`
lib/platform/src/linux/linux_x11keyboard.c	`0.00% <0.00%> (ø)`
lib/platform/src/linux/linux_clipboard.c	`74.28% <74.28%> (ø)`
lib/ui-widgets/src/textedit.c	`53.24% <95.83%> (+6.16%)`	:arrow_up:
lib/ui/src/widget_event.c	`62.04% <100.00%> (+1.58%)`	:arrow_up:
src/main.c	`63.28% <100.00%> (+0.22%)`	:arrow_up:
lib/util/src/rect.c	`49.65% <0.00%> (+0.34%)`	:arrow_up:
lib/ui/src/widget_paint.c	`74.71% <0.00%> (+0.57%)`	:arrow_up:
lib/text/src/textlayer.c	`65.94% <0.00%> (+0.67%)`	:arrow_up:
lib/ui/src/widget_diff.c	`95.94% <0.00%> (+1.35%)`	:arrow_up:
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b3a0a94...d5685e9. Read the comment docs.

WhoAteDaCake commented 2 years ago

I've not got the text back from clipboard to the textedit widget!

The approach is still quite basic and needs more handling, but it's looking promising

WhoAteDaCake commented 2 years ago

@lc-soft Could you take a look at the implementation?

Also, I'm a bit confused why some of my commit messages don't pass commit-lint-pr

WhoAteDaCake commented 2 years ago

I still need to add incremental copy pasting as well as test it.

Any thoughts on the best way to test the code?

WhoAteDaCake commented 2 years ago

@lc-soft In regards to callbacks: The reason we only store 1 callback is because it's destroyed as soon as it finished executing. The order of events is: CTRL+V -> LCUI_UseClipboard(widget, TextEdit_OnClipboardReady(action)) -> Register callback + widget -> Ask clipboard owner for text -> Function ends

SelectionNotify -> OnSelectionNotify -> Find existing callback -> Parse received text -> Call callback (TextEdit_OnClipboardReady(widget, text)) -> Call "paste" event on the widget with the provided text.

This is the order of execution and why the widget is passed at the moment. That's also why you don't see LCUI_HasClipboardText or LCUI_GetClipboardText, we don't always own the clipboard, so we don't know how it might change. In the worst case, it could happen that we run LCUI_HasClipboardText, then the clipboard text changes and when LCUI_GetClipboardText is ran, the lengths don't match up.

If you don't watch to pass the widget, we could maybe use LCUIWidget_GetFocus to get the focused widget? The we'd only need to pass text in a callback

The reason, why you see libraries like SDL and glfw have a synchronous version is because they have a while loop, where block the event loop until they receive the event. That's why I've used a callback system.

I hope that clarifies things

lc-soft commented 2 years ago

@lc-soft In regards to callbacks: The reason we only store 1 callback is because it's destroyed as soon as it finished executing. The order of events is: CTRL+V -> LCUI_UseClipboard(widget, TextEdit_OnClipboardReady(action)) -> Register callback + widget -> Ask clipboard owner for text -> Function ends ...

Ok, I suggest renaming the first parameter of LCUI_UseClipboard(), reason:

Parameter type void* tells the user that any type of data can be passed in, but parameter name widget can easily be mistaken for data that can only be passed in LCUI_Widget type.

lc-soft commented 2 years ago

I still need to add incremental copy pasting as well as test it.

Any thoughts on the best way to test the code?

Add file test/test_clipboard.c, and refer to similar test codes:

WhoAteDaCake commented 2 years ago

@lc-soft Is this better? It's now passing down LCUI_ClipboardText with wchar_t and size, I thought it might be useful for some cases. Though TextEdit_OnClipboardReady is now taking widget as a second argument.

lc-soft commented 2 years ago

@lc-soft Is this better? It's now passing down LCUI_ClipboardText with wchar_t and size, I thought it might be useful for some cases. Though TextEdit_OnClipboardReady is now not taking widget as a second argument.

I suggest to rename LCUI_ClipboardText to LCUI_Clipboard, and then update the definition of the structure:

typedef struct LCUI_ClipboardRec_ {
    LCUI_Graph image;
    wchar_t *text;
    size_t text_len;
} LCUI_ClipboardRec, *LCUI_Clipboard;

Reason: The clipboard may need to support copy and paste image in the future.

WhoAteDaCake commented 2 years ago

Would you expect me to add a new macro called LCUI_KEYCODE_V and LCUI_KEYCODE_C ? Or convert the e->key.code to char and check for 'C' or 'V'

Also, where can I find LCUI_GRAPH

lc-soft commented 2 years ago

Would you expect me to add a new macro called LCUI_KEYCODE_V and LCUI_KEYCODE_C ? Or convert the e->key.code to char and check for 'C' or 'V'

Also, where can I find LCUI_GRAPH

Use LCUI_KEY_V and LCUI_KEY_C.

https://github.com/lc-soft/LCUI/blob/b3a0a94a9594c5c9451a2a5b0c2a8b13e5146840/include/LCUI/input.h#L66-L85

The LCUI_Graph type has been renamed to pd_canvas_t.

WhoAteDaCake commented 2 years ago

@lc-soft I'm trying to start testing the code, however, I'm encountering some warnings:

test clipboard
warning: cannot match target(run-tests).add_files("test/run_tests.c") at ./test/xmake.lua:10
warning: cannot match target(run-tests).add_files("test/cases/*.c") at ./test/xmake.lua:10

Any tips?

WhoAteDaCake commented 2 years ago

I've added a copy paste test, does it look alright? Is there a need add additional tests?

lc-soft commented 2 years ago

@lc-soft I'm trying to start testing the code, however, I'm encountering some warnings:
test clipboard
warning: cannot match target(run-tests).add_files("test/run_tests.c") at ./test/xmake.lua:10
warning: cannot match target(run-tests).add_files("test/cases/*.c") at ./test/xmake.lua:10
Any tips?

In which directory did you run xmake run run tests?

WhoAteDaCake commented 2 years ago

@lc-soft I'm trying to start testing the code, however, I'm encountering some warnings:
test clipboard
warning: cannot match target(run-tests).add_files("test/run_tests.c") at ./test/xmake.lua:10
warning: cannot match target(run-tests).add_files("test/cases/*.c") at ./test/xmake.lua:10
Any tips?
In which directory did you run xmake run run tests?

I've ran it from the project run directory.

WhoAteDaCake commented 2 years ago

Thank you for your patience

lc-soft commented 2 years ago

Run tests took too long

WhoAteDaCake commented 2 years ago

Could it be because the test requires x11 clipboard to be available within the running machine and it's not there? That's the reason, why I've added that timeout Observer thread, I wasn't if it would be caught if something like this had happened.

Edit: No it seems like it didn't get to that point, I'll investigate this later today

lc-soft commented 2 years ago

Could it be because the test requires x11 clipboard to be available within the running machine and it's not there? That's the reason, why I've added that timeout Observer thread, I wasn't if it would be caught if something like this had happened.

Edit: No it seems like it didn't get to that point, I'll investigate this later today

I think you should add observer thread to the linux_x11clipboard.c file and pass empty data to the callback when the clipboard request times out.

lc-soft commented 2 years ago

I recommend that you add a default clipboard driver for use when X11 and Windows clipboard drivers are not available.

WhoAteDaCake commented 2 years ago

I think you should add observer thread to the linux_x11clipboard.c file and pass empty data to the callback when the clipboard request times out.

Should we maybe indicate if it failed in some sort of way as well? Since if empty text is returned, there is no way to know whether nothing is copied or it just failed.

I recommend that you add a default clipboard driver for use when X11 and Windows clipboard drivers are not available.

Where should i place this? Create a new folder in /lib/platform/src/ called headless or just place clipboard.c in /lib/platform/src/?

lc-soft commented 2 years ago

Should we maybe indicate if it failed in some sort of way as well? Since if empty text is returned, there is no way to know whether nothing is copied or it just failed.

Pass NULL to the callback to tell it that the clipboard request failed.

callback->action(NULL, callback->arg);

Where should i place this? Create a new folder in /lib/platform/src/ called headless or just place clipboard.c in /lib/platform/src/?

Please ignore my last comment because it's a bit complicated to implement.

An easier way to fix build failures in Windows is to add functions such as LCUI_UseClipboard without their implementation code.

void LCUI_UseClipboard(LCUI_ClipboardAction action, void *arg)
{
    // Empty
}

WhoAteDaCake commented 2 years ago

I've added an observer thread, in regards to testing the clipboard, how should I go about testing it? Thanks.

lc-soft commented 2 years ago

I've added an observer thread, in regards to testing the clipboard, how should I go about testing it? Thanks.

I think the test method in the current test_clipboard.c file is ok.

WhoAteDaCake commented 2 years ago

But since there is no clipboard access in the CI pipeline, it won't work right? As we'd get is a timeout for when we request to own the clipboard :thinking: . I'll investigate this further

WhoAteDaCake commented 2 years ago

@lc-soft

So the reason it hangs it's because inside linux_clipboard.c LCUI_UseClipboard implementation is empty when X11 is not used, so the "change" event within the test never gets triggered.

I could try and add an In file clipboard for Linux within the linux_clipboard.c, which would be used when x11 is not available.

lc-soft commented 2 years ago

@lc-soft

So the reason it hangs it's because inside linux_clipboard.c LCUI_UseClipboard implementation is empty when X11 is not used, so the "change" event within the test never gets triggered.

I could try and add an In file clipboard for Linux within the linux_clipboard.c, which would be used when x11 is not available.

Try binding the "paste" event handler

lc-soft commented 2 years ago

But since there is no clipboard access in the CI pipeline, it won't work right? As we'd get is a timeout for when we request to own the clipboard thinking . I'll investigate this further

Yes, I will test this pull request with WSL in a few days.

WhoAteDaCake commented 2 years ago

Try binding the "paste" event handler

The paste event won't get called either, since LCUI_UseClipboard never sets the callback.

WhoAteDaCake commented 2 years ago

Does that work @lc-soft ?

WhoAteDaCake commented 2 years ago

I think it would be useful to have this kind of conversion between (char -> w_char) and (w_char -> char) extracted as a utility function, since it's easy to mess up, and it can be useful in many places

lc-soft commented 2 years ago

Does that work @lc-soft ?

Please try to replace #include "config.h" with #include "../config.h" in directory lib/platform/src/linux/

WhoAteDaCake commented 2 years ago

Does that work @lc-soft ?

Please try to replace #include "config.h" with #include "../config.h" in directory lib/platform/src/linux/

I think the failing tests aren't related to the config right?

WhoAteDaCake commented 2 years ago

  root width 320px
    √ $('.example')[0].box.border == (10, 10, 286, 604)
    √ $('.example')[1].box.border == (10, 624, 286, 304)
    × $('.example')[2].box.border == (10, 938, 286, 280)
      AssertionError: (10, 938, 286, 242) == (10, 938, 286, 280)
      + expected - actual

      - (10, 938, 286, 242)
      + (10, 938, 286, 280)

    × $('.example')[3].box.border == (10, 1228, 286, 732)
      AssertionError: (10, 1190, 286, 732) == (10, 1228, 286, 732)
      + expected - actual

      - (10, 1190, 286, 732)
      + (10, 1228, 286, 732)

    × $('.example')[4].box.border == (10, 1970, 286, 504)
      AssertionError: (10, 1932, 286, 504) == (10, 1970, 286, 504)
      + expected - actual

      - (10, 1932, 286, 504)
      + (10, 1970, 286, 504)

    × $('.example')[5].box.border == (10, 2484, 286, 254)
      AssertionError: (10, 2446, 286, 254) == (10, 2484, 286, 254)
      + expected - actual

      - (10, 2446, 286, 254)
      + (10, 2484, 286, 254)

lc-soft commented 2 years ago

  root width 320px
    √ $('.example')[0].box.border == (10, 10, 286, 604)
    √ $('.example')[1].box.border == (10, 624, 286, 304)
    × $('.example')[2].box.border == (10, 938, 286, 280)
      AssertionError: (10, 938, 286, 242) == (10, 938, 286, 280)
      + expected - actual

      - (10, 938, 286, 242)
      + (10, 938, 286, 280)

    × $('.example')[3].box.border == (10, 1228, 286, 732)
      AssertionError: (10, 1190, 286, 732) == (10, 1228, 286, 732)
      + expected - actual

      - (10, 1190, 286, 732)
      + (10, 1228, 286, 732)

    × $('.example')[4].box.border == (10, 1970, 286, 504)
      AssertionError: (10, 1932, 286, 504) == (10, 1970, 286, 504)
      + expected - actual

      - (10, 1932, 286, 504)
      + (10, 1970, 286, 504)

    × $('.example')[5].box.border == (10, 2484, 286, 254)
      AssertionError: (10, 2446, 286, 254) == (10, 2484, 286, 254)
      + expected - actual

      - (10, 2446, 286, 254)
      + (10, 2484, 286, 254)

This problem can be ignored because the test program loaded different fonts in the test environment than in my system, and rendered text with these fonts was smaller, resulting in a layout that was not as expected.

WhoAteDaCake commented 2 years ago

Doing the last check of trying to figure out why the test is failing and then I can confirm

WhoAteDaCake commented 2 years ago

Just as a final note. Is #USE_LIBX11 always set? Even in CI environment it seems to be set, that's why after I added local data to linux_clipboard.c it hung because I added else to the macro.

WhoAteDaCake commented 2 years ago

Last fix: there is a memory leak, since after clipboard text + clipboard_data is sent, it's not freed. Which part of code should free it? textedit or clipboard

WhoAteDaCake commented 2 years ago

Or potentially, I can store that data and free the previous allocation on the next ask for clipboard, this way there is no need to try and communicate if multiple listeners are created for the paste event, what do you think?

lc-soft commented 2 years ago

Just as a final note. Is #USE_LIBX11 always set? Even in CI environment it seems to be set, that's why after I added local data to linux_clipboard.c it hung because I added else to the macro.

Yes, it is, but you should handle the case where #USE_LIBX11 is not set.

lc-soft commented 2 years ago

Last fix: there is a memory leak, since after clipboard text + clipboard_data is sent, it's not freed. Which part of code should free it? textedit or clipboard

clipboard

WhoAteDaCake commented 2 years ago

Just as a final note. Is #USE_LIBX11 always set? Even in CI environment it seems to be set, that's why after I added local data to linux_clipboard.c it hung because I added else to the macro.

Yes, it is, but you should handle the case where #USE_LIBX11 is not set.

In that case, we should not set it in environments like CI, as the handling would be exactly the same as if LCUI_GetAppId() != LCUI_APP_LINUX_X11

WhoAteDaCake commented 2 years ago

Or I can just extract it to a function and make sure both variants call it

WhoAteDaCake commented 2 years ago

The tests are now passing, if code is looks ok to you, please merge

WhoAteDaCake commented 2 years ago

Thank you for all the guidance!

lc-soft commented 2 years ago

@WhoAteDaCake I made some changes to make the clipboard tests work on Windows.

https://github.com/lc-soft/LCUI/blob/v3.0-dev/lib/platform/src/clipboard.c

lc-soft / LCUI

feat(platform): add clipboard support #271

Chore

Features

Bug Fixes

Contributors

Codecov Report