Shared clipboard feature improvements

[Radu Cristescu]

I always wanted to copy on one laptop and paste on the other, and now I found this feature in glipper 2.1 and it's really cool. By the looks of it, this feature isn't production ready yet, but here is a patche that makes the feature work properly (except for the preferences dialog, I'm using clipboard sharing with this patch right now).

What the patch does:

  * Fixes for shared clipboards
    - Base64 encode the clipboard data to avoid nasty surprizes when the
      encryption produces NUL bytes in the middle of the result;
    - Enable GDK threads and do proper locking to avoid deadlock when setting
      the clipboard text from a StringListener thread;
    - Detect loops like in plugins/newline.py

Now... I'm not a Python programmer at all, so I may have missed a few gotchas. But at least it works :) - "for now"(tm)

[Laszlo Pandy (laszlok)]

Is it at all possible to not have the lock handling code in Clipboard.py? I'm not sure if that's the best place for it.

[Radu Cristescu (radu.c)]

Sorry about the second patch: I put locks in the wrong place.

[Radu Cristescu (radu.c)]

I put the locks around GDK GUI calls as I saw recommended at these links:

http://stackoverflow.com/questions/3212010/why-does-my-pygtk-application-crash-when-copying-text-on-a-clipboard

http://faq.pygtk.org/index.py?file=faq20.006.htp&req=show

I didn't give it much thought.

[Radu Cristescu (radu.c)]

I still had deadlocks with my previous patch and I could find a reasonable way to fix it using locks, although that's how I would have preferred to do it. Given that, I followed method 1 in the PyGTK FAQ and came up with the following patch, which works a lot better. I think there's a small chance of losing data, but there's also a high chance that my way of testing just hit the "extension" code, as I was just randomly selecting test on my screen and checking the other screen after 10 copies to see if I have 10 copies.

[Radu Cristescu (radu.c)]

Previous comment should have said "I couldn't find a reasonable way to fix it using locks".

[Radu Cristescu (radu.c)]

In the new patch I also put a gratuitous SO_REUSEADDR, because I was starting to get annoyed by "address already in use" errors. :)

[Laszlo Pandy (laszlok)]

Maybe you should create your own branch for this, and then I can merge the changes. I feel like this is going to take more than one patch to fix.

Since you said "this feature isn't production ready yet" I've been thinking of how it should actually work. I think there should be one network consumer thread, and we make use of Python's synchronized Queue. The network.py plugin would spawn the consumer thread, and also register a gobject.timeout_add() on the main thread, which would check the queue and set the clipboard text.

I believe this would solve all these thread and GTK issues and avoid changing any code glipper code (the changes would contained in the plugin).

Thoughts?

[Radu Cristescu (radu.c)]

As I said earlier, I'm not a Python programmer at all. So far, all I did was hack the existing code so it works, with plenty of Googling and RTFM-ing - and that's because I always wanted to copy from one laptop to another (usually youtube links from a chat window on a low powered laptop to a more powerful laptop where I don't want to run the chat application).

If what you say is what I think you say, the idea is actually good. The network.py plugin can even stay the way it is now for the most part (and can be changed later), with the difference that it will push incoming clipboard messages to the synchronized Queue and let the main thread pick them up and update the menu. That change wouldn't even be a big deal if I knew Python well enough.

Unfortunately, at least one other plugin demonstrates code that needs the intrusive changes in glipper: newline.py. I don't know about the rest, but this plugin alters the history directly too. They'd all have to be changed, which can be quite a lot of work. The way I did it, all of them go through the idle queue of the main thread without changes. May not be the prettiest code, but it works, and it's a lot better than what was there originally.

On creating my own branch: I don't know Bazaar either. I'd have to learn it, which would take... about two hours after I wake up :) if necessary. I could probably use one anyway, since right now I'm not using any code versioning for these patches, which kind of defeats the purpose of Bazaar, and I found a slight glitch in the last patch which could cause trouble if the data came with the "\0" packed in (which was the thing I was trying to fix with the while True loop to begin with), so I'll post an updated patch just for that in a minute.

[Laszlo Pandy (laszlok)]

Does newline.py require the same thread locking or some other intrusive changes?

I can help you out with Bazaar if you are on IRC somewhere. It also might be faster to discuss these changes on chat as well.

There might still be changes needed to glipper if each plugin is duplicating the same code to protect against recursion and for threads. But I still think we should fix the plugins individually, and once we have the plugins working, we will be able to see more clearly what changes would be useful in glipper core to reduce code duplication.

[Radu Cristescu (radu.c)]

Let's just say that newline.py is a _very_ lightweight version of the network.py plugin - just a lot worse. It takes your last entry, appends \n to it and puts it back. grow.py seems to do a similar thing (it's what I called "extension" earlier). They stand a chance of locking up glipper as well (although I haven't seen them do this yet, and I don't know what the difference is). newline.py is probably not used by anyone, grow.py goes through the history rather than the clipboard, nopaste.py only reads the first entry of the history and posts it to a pastebin-like site and opens a browser with the link (it doesn't do that for me though, just reading the code about what it should do)... so not many chances of deadlocking glipper there.

There may be some secret sauce in "emit" which can cause the actual clipboard action to happen on the main thread instead of the emitting thread, explaining why I haven't seen deadlocks before (I always used grow.py) and that makes me wonder why is add_history_item going straight for the clipboards while all the others go through the history. I have to say I don't know Gobject either :) I only know a bit of Qt, and the concepts seem similar (just different programming language).

I'm on IRC on Freenode, channel #linuxmce-devel, my nick there is Uplink_ (with the underscore), but I do weird hours, so you never know if I'm around (for example, today I've been awake for 20 hours and I'd better get to bed). I can learn Bazaar from the documentation. Already committed the latest patch in my local branch, and if I'll start doing heavy work, I'll probably put a copy on Launchpad too. But right now I like the last patch the way it is :D - Which is quite good given the fact that I don't know anything about the language (Python) or the framework (Gobject, GTK, GDK) and it's the first time I dived into glipper code.

[Laszlo Pandy (laszlok)]

But newline.py doesn't use separate threads. Nor does grow.py. Grow.py doesn't even actually set the clipboard text either. So why would those cause a deadlock?

Only nopaste.py and network.py use threads, and nopaste.py only uses a thread to post to the internet. So if network.py is the only one which calls Gtk code from a separate thread, why would any of the others cause deadlocks?

[Radu Cristescu (radu.c)]

True. You can tell I'm sleepy :)

[Radu Cristescu (radu.c)]

So OK, in that case my entire argument above fails, and the best way is to do that synchronized Queue. The rest of the plugins won't be affected, and there's no extra work adapting them. I guess I can go the extra mile and adapt the changes to follow this idea, but after I'm awake again.

[Radu Cristescu (radu.c)]

I changed the code to use Queue objects (see associated branch). In the process I found a few other issues with communication ordering, so the branch includes changes for that too. The only change in glipper itself is to initialize threading.

[Laszlo Pandy (laszlok)]

I am writing a replacement for the network.py plugin which:
 * Uses only one new thread to manage all network connections.
 * Uses synchronized queues to communicate with the Gtk mainloop thread.
 * Uses avahi to simplify connecting to other computers (no need to input IP).

It is current called new_network.py in its own branch:
https://code.launchpad.net/~laszlok/glipper/avahi-network-plugin

The plugin in currently not functional because the UI has not been written yet. However most of the network and avahi code is there.