[linux] support for simple xselection pasting

[sergiomb2]

From: Matthias Kortstiege mkortstiege@kodi.tv
Date: Sat, 4 Jul 2015 16:51:06 +0200
Subject: [PATCH] [linux] support for simple xselection pasting

in http://forum.kodi.tv/showthread.php?tid=231227 says to patch kodi with
mkortstiege@62e56b7

I test it works and no regressions noted . Kodi don't have copy and paste working on Linux !

[FernetMenta]

X11 does already poll XNextEvent. Makes no sense to do the same at a different location. I also has a main window, no need to create an additional one.

[sergiomb2]

Hello, I just want paste clipboard into kodi , it is very useful and this patch works , I will try make a better patch but if you suggest on coding, by examples, it is very welcome .
Thanks.

[FernetMenta]

check out https://github.com/xbmc/xbmc/blob/master/xbmc/windowing/WinEventsX11.cpp#L322
there is no need for an addition window

[sergiomb2]

Patch updated , works for me , I don't know what should be used in window var , if m_glWindow or m_mainWindow , or if I should create a new one in line fa1b9c7#diff-f78301169cdbe1580bc520b8848e536cR1082 and line fa1b9c7#diff-f78301169cdbe1580bc520b8848e536cR1096

[FernetMenta]

hmm, you seem to have ignored my comments: no additional window, no additional XNextEvent

[sergiomb2]

Hi, I done another patch sergiomb2@cb0a44a
The main question is where should I put this 2 lines ? :

Atom utf8_string = XInternAtom(m_dpy, "UTF8_STRING", False);
XConvertSelection(m_dpy, XA_PRIMARY, utf8_string, None, m_glWindow, CurrentTime);

My option was add XConvertSelection to CWinSystemX11::NotifyAppFocusChange when it is gaining focus, in my tests seems that works well .

git master updated, ready to be merged !
Thanks.

[FernetMenta]

check out what SDL does: http://www.libsdl.org/tmp/SDL/src/video/x11/SDL_x11clipboard.c
this can be easily ported to Kodi.

[sergiomb2]

I will try change and test XA_PRIMARY to XA_CLIPBOARD.
The feature that is very important to have is "simple xselection pasting", have full feature of copy and paste working seems much more complicated and I haven't time or acknowledgment to do that , also don't think even that is practical, after analyze one textbox in kodi under Linux I see that can't select the text in the box , so I also can't copy or cut the text , but paste text from clipboard is possible and the unique feature that I like to have and might be useful for kodi users , so the question is, one simple patch like this can be upstreamed or I just use it in my packages ?

[sergiomb2]

Hello
I take a look in projects:
https://github.com/astrand/xclip
https://github.com/edrosten/x_clipboard
http://www.vergenet.net/~conrad/software/xsel
specs:
https://specifications.freedesktop.org/clipboards-spec/clipboards-latest.txt
and some code:
https://github.com/ccxvii/snippets/blob/master/x11clipboard.c
https://github.com/myint/pygame/blob/master/src/scrap_x11.c
https://git.enlightenment.org/apps/eterm.git/tree/src/startup.c

I found one method (EnableTextInput) where I could put the XConvertSelection , I also could read just CLIPBOARD instead XA_PRIMARY, so now for me my patch looks good to be applied, all code seems in right place.
And it is working applied on Jarvis 16.1 .

[FernetMenta]

This code may work, somehow, but it is rather hacky. We had too many of those "it works somehow hacks" in the past that made architecture decay. There are many locations in this PR that need rework.
I already told you that we won't blindly call XConvertSelection. Now you moved it from one location that was wrong to another that is wrong too. Why don't you follow my advice any look into SDL code?

[sergiomb2]

It looks good to me have XConvertSelection in EnableTextInput ... , today I understand better SDL code (after yesterday read much documentation) , SDL code do all in same function , after XConvertSelection, do something that I don't know how to do, which is :

    waitStart = SDL_GetTicks();
    videodata->selection_waiting = SDL_TRUE;
    while (videodata->selection_waiting) {

SDL code waits for selection ... how we may do this ( I will google it) .

About "it works somehow hacks" , I understand you very well , but know I don't think my patch is a hack , when X enables a text input we call XConvertSelection , looked good to me .
Thanks .

[FernetMenta]

About "it works somehow hacks" , I understand you very well , but know I don't think my patch is a hack , when X enables a text input we call XConvertSelection

g_Windowing.EnableTextInput is called by Kod's edit control when it gets focus. Nothing related to X.

[FernetMenta]

SDL code waits for selection ... how we may do this ( I will google it) .

exactly the same way. Call MessagePump for a given time.

[sergiomb2]

SDL code waits for selection ... how we may do this ( I will google it) .

exactly the same way. Call MessagePump for a given time.

Done ! , now I think it it by the book.

Thanks.

[FernetMenta]

why don't you call MessagePump as I suggested?

[sergiomb2]

why don't you call MessagePump as I suggested?
OK , Using MessagePump it is the correct way , what do you think now ?

[sergiomb2]

https://www.jwz.org/doc/x-cut-and-paste.html
in 2002 Cut buffers are officially obsolete.

I don't know how translate : selection = X11_GetSDLCutBufferClipboardType(display); and
selection = X11_XInternAtom(display, "SDL_SELECTION", False);

I can add checks to atom like this:

   Atom utf8_string = XInternAtom(m_dpy, "UTF8_STRING", False);
+  if (utf8_string == None) {
+    return "";
+  }
   Atom clipboard = XInternAtom(m_dpy, "CLIPBOARD", False);
+  if (clipboard == None) {
+    return "";
+  }

owner = X11_XGetSelectionOwner(display, XA_CLIPBOARD);
if owner is null instead fall back to X11 of 2000 or return "" (which means we don not support it)
if owner is window I don't get it, X11_GetSDLCutBufferClipboardType !? , CutBuffer in 2002 was already obsoleted.
if not set owner = window and do X11_XConvertSelection, so for me we we have atoms we do just this case.

of course you need to adapt MessagePump that is listens to the event triggered by XConvertSelection

Also I'm not seeing what we need , MessagePump returns true or false , if we process one event ...

[FernetMenta]

you don't want to wait for any event but for a specific one. hence you need to modify MessagePump to signal this specific event. while waiting for this event MessagePump will deliver other events to application

[sergiomb2]

Keep it simple, we just need call MessagePump once , when MessagePump returns all events are processed, I think.
if XGetWindowProperty returns Success we can copy result and loop until retrieve all content .
if XGetSelectionOwner returns null if no selection or there is no owner for the selection (ancient X ?!)
Now patch looks that works with synchronized code and we verify if X11 support Atoms and other stuff like XGetSelectionOwner , if not we simply return an empty string like happens before this patch.
Best regards.

[FernetMenta]

keep is simple is good, but first it needs to be correct.

[sergiomb2]

we just need call MessagePump once , when MessagePump returns all events are processed, I think.

After test it , I saw that we need wait for one MessagePump positive.
If by a race condition, which is process one event, empty the event queue and SelectionNotify was not called, selection will not be update , but don't do any harm . The same will happen if SelectionNotify take more 200ms ...

About X11_XGetSelectionOwner and when owner == window , for what I understood this is just applicable on SDL or other cases where windows have selection saved as one property , which is not applicable to X11.

[FernetMenta]

You don't get around modifying MessagePump. It does not return true an any event and you current code just times out every time.

[sergiomb2]

Now patch verify:

1. if X11 not support Atoms and other stuff like XGetSelectionOwner, we simply return an empty string like happens before this patch.
2. if XGetWindowProperty returns Success, we copy result and loop until retrieve all content .
3. if XGetSelectionOwner returns null, means: no selection or there is no owner for the selection (ancient X ?!), we return an empty string.
4. We need wait for MessagePump return true , I add a log line to see and count how many times MessagePump is called [1]

Adding catch SelectionNotify and return true like this :

  case SelectionNotify:
    ret = true;
    break;

Without catch SelectionNotify, MessagePump was called 10 or 11 times , with SelectionNotify catch, MessagePump is called only 2 times !
I personally would like not change CWinEventsX11Imp::MessagePump, because this function can be called in other situations that I don't control. But set return true when SelectionNotify is called, seems to me, harmless, this is the only doubt I have now , I can't find a way to have the code in a synchronized way.
Anyway, I checked where MessagePump is called in kodi and return code is always ignored. so seems be safe the change in MessagePump function.

Best regards.
[1] CLog::Log(LOGNOTICE, "GetClipboardText calls MessagePump count = %d ", count);

[FernetMenta]

Getting closer :) but I see no need for guessing ind praying that things go right now and in future. We can set an event/flag on SelectionNotify and wait on that event in WinSystemX11