Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature #879

iloveeclipse · 2023-11-10T14:10:34Z

The utf-8 content should be same as in UTF8_STRING, it just should use different label/type id.
The change in ClipboardProxy.setData() was non functional and makes no sense, it only leaks memory/wastes time

1) The utf-8 content should be same as in UTF8_STRING, it just should use different label/type id. 2) The change in ClipboardProxy.setData() was non functional and makes no sense, it only leaks memory/wastes time See eclipse-platform#863 See eclipse-platform#851

iloveeclipse · 2023-11-10T14:16:10Z

@the-snowwhite : please test this implementation with your original use case in #851.

github-actions · 2023-11-10T14:20:55Z

Test Results

    299 files ±0     299 suites ±0 6m 18s ⏱️ +20s
  4 095 tests ±0   4 087 ✔️ ±0   8 💤 ±0 0 ❌ ±0
12 197 runs ±0 12 124 ✔️ ±0 73 💤 ±0 0 ❌ ±0

Results for commit 5c6a7a7. ± Comparison against base commit 5193ab5.

the-snowwhite · 2023-11-10T14:43:05Z

@iloveeclipse
Your statement here is a misunderstanding.(verified bu the eclipse debugger)

The utf-8 content should be same as in UTF8_STRING, it just should use different label/type id.

The change in ClipboardProxy.setData() was non functional and makes no sense, it only leaks memory/wastes time

See #863 See #851

I have fully tested the functionality of #863 wia functional testing in the eclipse debugger running in a wayland session.
with only the changes in TextTransfer.java the debugger responded to no breakpoints in javaToNative (in TextTransfer.java)
and no data was presented to the wayland clipboard.

Then after introducing the 3 lines in ClipboardProxy.java the debugger now stopped in javaToNative and data was presented to the wayland clipboard.

Please suggest functional code changes in functional code instead of abstract programming lingo the is hard for me to understand.
I'm an intuition based functional procedural programmer with a gift for code and bugfixes but i do not understand OOP or OOP lingo.
OK ?

the-snowwhite · 2023-11-10T15:14:21Z

@the-snowwhite : please test this implementation with your original use case in #851.

OK I hope it works I like the much more simple style than what chatgpt4 beaten with a (stick and large scissors) could provide...

iloveeclipse · 2023-11-10T15:22:31Z

I have fully tested the functionality of #863 wia functional testing in the eclipse debugger running in a wayland session.
with only the changes in TextTransfer.java the debugger responded to no breakpoints in javaToNative (in TextTransfer.java)
and no data was presented to the wayland clipboard.

What I'm asking you now: please test this patch with Wayland! Not what you might have tested before in some debug session!

If it is not working under Wayland, it means, the code you've added in ClipboardProxy.setData() should only be called under Wayland session (if it the right place to do it at all, which I highly doubt here), because now under X11 I see that we call this code three times and it is not needed at all under X11!

The use case (as far as I could understand it, because it is not clearly described) was:

running in both gtk v3.24.33 and gtk v3.24.38 it is possible to paste into eclipse from all applications that only present
RFC-1341 types (so far only text/plain;charset=utf-8) in the clipboard under a wayland session.
I have also just tested pasting the danish special characters åæø and that went fine also.

If I understood it right, the text copied to clipboard from Eclipse should be able paste to "kate" editor under Wayland.

Note: it is not the ClipboardProxy.setData() that should call javaToNative(), but ClipboardProxy.getFunc(), at least under X11, so if under Wayland it is differently, you should look why ClipboardProxy.getFunc() doesn't call javaToNative or why ClipboardProxy.getFunc() is not callled.

I have no Wayland, so I can't check, but current code state is definitely wrong.

@mickaelistria, @akurtakov, @trancexpress : could you please check this PR & use case, if you have Wayland?

If this PR doesn't work under Wayland, I would propose a PR reverting #863 for RC1 as it definitely does wrong things on X11.

the-snowwhite · 2023-11-10T15:31:39Z

@iloveeclipse

What I'm asking you now: please test this patch with Wayland! Not what you might have tested before in some debug session!

Yes i'm just having some issues as my main eclipse installation for SWT building has crapped out showing over 861 errors
and not being able to run the clipboard example .
Luckily I have a Kubuntu 22.04 VM that has a working Eclipse build setup, however now I'm having some issues installing wayland so it's slow progress. I hope your PR works as its a much more simple solution (stated more verbosely in other comment).

the-snowwhite · 2023-11-10T15:34:06Z

@iloveeclipse
Btw the audience for the paste fix is ubuntu 22.04 LTS users that have gtkv3.23.33(the one without the gtk paste fix).

mickaelistria · 2023-11-10T16:57:40Z

@the-snowwhite So do I get it right you successfully tested that patch and are OK with it being merged?

iloveeclipse · 2023-11-10T17:38:15Z

@the-snowwhite : once again: can you clearly answer to the question: have you successfully tested that patch and are OK with it being merged?

@mickaelistria : can you please test this patch - as obviously you were able test the original one?

the-snowwhite

@iloveeclipse
The title is mis leading
This is a functional change for an already functional verified and merged PR
Will you please wait til after the freeze ?

iloveeclipse · 2023-11-10T17:40:36Z

Will you please wait til after the freeze ?

No, because original patch is NOT OK.

Can you please answer to the question have you successfully tested that patch and are OK with it being merged?

the-snowwhite · 2023-11-10T18:05:27Z

@iloveeclipse
@mickaelistria
The original intention of testing required that the buxfix was able to run on 2012-09_R So next logical test step will be
to cherrypick the #879 to r_29_Maintanance and test via the
#858
procedure.
However if that works next logical step will be I have to figure out which branch corresponds to the most recent binaries
created by the I-build (I think @mickaelistria gave me the info yesterday ?).
And then download the binaries from: here ?
https://www.eclipse.org/downloads/packages/2023-03m3
and test the 3 binaries I can produce in the plugin folder ...
oh wait the filename is
eclipse-inst-jre-linux64.tar.gz
Hmmm will ruin my current 2012-09 installs ?
Ill start on the test procedure but give me a few hours or so
OK ?

mickaelistria

Using the ClipboardExample
Without patches from #863 , I can copy from StyledText to Qt widget (I'm using Carla application as example of a KDE/Qt application) but I cannot copy from Carla/Qt/Kde... to StyledText.
With current master including patches from #863, I can copy/paste in both directions (that what I tested before merging)
With this patch, I can also copy in both directions.

So I'm totally in favor of merging this patch, which achieves the same result with simpler code and removing some extra unnecessary processing.

Thanks @iloveeclipse for the polishing here.

iloveeclipse · 2023-11-10T18:25:20Z

Thanks @mickaelistria, merging.

iloveeclipse requested review from sravanlakkimsetti and akurtakov as code owners November 10, 2023 14:10

This was referenced Nov 10, 2023

Wayland qt kde clipboard fix #863

Merged

Cosmetic change only Fix indentation problem introduced in c7549d9 #875

Closed

iloveeclipse mentioned this pull request Nov 10, 2023

Revert changes from #863 #880

Closed

the-snowwhite suggested changes Nov 10, 2023

View reviewed changes

iloveeclipse changed the title ~~Cleanup implementation of TEXT_PLAIN_UTF8 transfer feature~~ Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature Nov 10, 2023

mickaelistria approved these changes Nov 10, 2023

View reviewed changes

iloveeclipse merged commit a90888b into eclipse-platform:master Nov 10, 2023
10 of 11 checks passed

iloveeclipse deleted the issue_851_followup branch November 10, 2023 18:26

This was referenced Nov 11, 2023

A suggestion for a more simple mostly automated way to test a patch. #884

Open

Test implemention of #858 suggested method #890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature #879

Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature #879

iloveeclipse commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023 •

edited

Loading

the-snowwhite commented Nov 10, 2023

mickaelistria commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023 •

edited

Loading

the-snowwhite left a comment

iloveeclipse commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023

mickaelistria left a comment

iloveeclipse commented Nov 10, 2023

Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature #879

Fix wrong implementation of TEXT_PLAIN_UTF8 transfer feature #879

Conversation

iloveeclipse commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023

github-actions bot commented Nov 10, 2023

Test Results

the-snowwhite commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023 • edited Loading

the-snowwhite commented Nov 10, 2023

mickaelistria commented Nov 10, 2023

iloveeclipse commented Nov 10, 2023 • edited Loading

the-snowwhite left a comment

Choose a reason for hiding this comment

iloveeclipse commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023

mickaelistria left a comment

Choose a reason for hiding this comment

iloveeclipse commented Nov 10, 2023

the-snowwhite commented Nov 10, 2023 •

edited

Loading

iloveeclipse commented Nov 10, 2023 •

edited

Loading