Bug 17267 - [GTK] Primary selection/clipboard support
Summary: [GTK] Primary selection/clipboard support
Status: RESOLVED DUPLICATE of bug 25685
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2008-02-09 19:21 PST by Alp Toker
Modified: 2009-05-12 10:14 PDT (History)
8 users (show)

See Also:


Attachments
Attempt at primary clipboard support (1.08 KB, patch)
2008-06-22 05:09 PDT, Christian Dywan
oliver: review-
Details | Formatted Diff | Diff
Primary clipboard copy support (3.94 KB, patch)
2008-09-10 08:32 PDT, Alp Toker
no flags Details | Formatted Diff | Diff
Copy link locations to the primary selection (4.33 KB, patch)
2008-10-27 15:47 PDT, Sergio García-Cuevas González
no flags Details | Formatted Diff | Diff
Preliminary primary selection paste support (2.25 KB, patch)
2009-01-28 15:46 PST, Sergio García-Cuevas González
gns: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alp Toker 2008-02-09 19:21:28 PST
We already have clipboard support, but it doesn't work with the primary clipboard on X.

We need to support

  (a) selections for pasting from WebKit into other applications
  (b) middle-click for pasting from the primary clipboard into editable WebKit areas

On platforms without a primary clipboard, GTK+ automatically makes this a no-op, so we don't need to add any conditional/X-specific code to support this.
Comment 1 Christian Dywan 2008-06-22 05:09:52 PDT
Created attachment 21872 [details]
Attempt at primary clipboard support

I do not know too well about WebCore's clipboard support, but at some point I really needed primary clipboard support.

So here is my attempt. It works for all I want.
Comment 2 Oliver Hunt 2008-06-22 05:35:05 PDT
Comment on attachment 21872 [details]
Attempt at primary clipboard support

r-, the shouldBlah methods are callbacks made to the api to query whether something is allowed, not that is has already happened.

EditorClient methods are (by and large) just present to allow WebCore's behaviour to be controlled through the platforms API, so all these methods should just map through to your API implementation.

The WebKit/GTK API's default implementation for didChangeSelectedRange (or whatever, definitely not shouldChangeSelectedRange) would probably be something along these lines.
Comment 3 Alp Toker 2008-08-11 16:26:50 PDT
The performance hit/memory fragmentation of converting HTML to text every time a large selection is changed would be too great, and would rule out the possibility of content negotiation (copy and paste as HTML, text, rich text, Pango markup etc.)

I think we need to avoid copying the inner text of the selection every time it changes, instead responding to a primary selection request when it happens (Qt does the former and I don't like it.)
Comment 4 Alp Toker 2008-09-10 08:32:50 PDT
Created attachment 23321 [details]
Primary clipboard copy support

The behaviour of this patch is intended to match GtkTextView (which is slightly different to the selection support in Firefox and OpenOffice and fits in better with GTK+ applications).
Comment 5 Alp Toker 2008-09-10 13:32:09 PDT
Comment on attachment 23321 [details]
Primary clipboard copy support

Landed in r36323. Still need paste support to close this bug.
Comment 6 Sergio García-Cuevas González 2008-10-27 15:47:21 PDT
Created attachment 24698 [details]
Copy link locations to the primary selection

This patch makes WebCore::Pasteboard::writeURL use the primary selection as well as the clipboard selection.
Comment 7 Alp Toker 2008-10-30 13:41:09 PDT
(In reply to comment #6)
> Created an attachment (id=24698) [edit]
> Copy link locations to the primary selection
> 
> This patch makes WebCore::Pasteboard::writeURL use the primary selection as
> well as the clipboard selection.
> 

Thanks, this is a reasonable feature to add. I'll review this one in a bit. There are a few interactions that need to be checked before getting this in.
Comment 8 Jan Alonzo 2008-11-29 23:52:52 PST
Comment on attachment 24698 [details]
Copy link locations to the primary selection

Alp to review..
Comment 9 Alexander Butenko 2009-01-12 21:36:56 PST
'Primary clipboard copy support' is already landed. i applied 'Copy link locations to the primary selection' patch but still cant paste nothing to editable areas in webkit.
Comment 10 Sergio García-Cuevas González 2009-01-28 15:46:33 PST
Created attachment 27129 [details]
Preliminary primary selection paste support

Here is my attempt to support middle-click pasting.  The target editable area must be selected, so it only works when the selection comes from an external application or when a clipboard manager is running (and then the selection comes from an external application, too).
Comment 11 Sergio García-Cuevas González 2009-01-28 15:56:00 PST
Comment on attachment 27129 [details]
Preliminary primary selection paste support

>Index: WebKit/gtk/ChangeLog
>===================================================================
>--- WebKit/gtk/ChangeLog	(revision 40327)
>+++ WebKit/gtk/ChangeLog	(working copy)
>@@ -1,3 +1,13 @@
>+2009-01-28  Sergio García-Cuevas <sergio_gcg@hotmail.com>
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=17267
>+
>+        Implement primary selection support (pasting into editable regions).
>+
>+        * WebCoreSupport/EditorClientGtk.cpp:
>+        (WebKit::EditorClient::buttonPressed):
>+        (WebKit::EditorClient::EditorClient):
>+
> 2009-01-27  Brady Eidson  <beidson@apple.com>
> 
>         Reviewed by Dan Bernstein
>Index: WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
>===================================================================
>--- WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	(revision 40327)
>+++ WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp	(working copy)
>@@ -37,6 +37,23 @@ using namespace WebCore;
> 
> namespace WebKit {
> 
>+static void buttonPressed(GtkWidget* widget, GdkEventButton* event, EditorClient* client)
>+{
>+    if (event->button == 2 && event->type == GDK_BUTTON_PRESS) {
>+        Frame* frame = core(client->m_webView)->focusController()->focusedOrMainFrame();
>+        Editor* editor = frame->editor();
>+        if (editor->canPaste()) {
>+            GtkClipboard* clipboard = gtk_widget_get_clipboard(GTK_WIDGET(client->m_webView), GDK_SELECTION_PRIMARY);
>+            gchar* utf8 = gtk_clipboard_wait_for_text(clipboard);
>+            if (utf8) {
>+                String text = String::fromUTF8(utf8);
>+                g_free(utf8);
>+                editor->insertText(text, 0);
>+            }
>+        }
>+    }
>+}
>+
> static void imContextCommitted(GtkIMContext* context, const gchar* str, EditorClient* client)
> {
>     Frame* targetFrame = core(client->m_webView)->focusController()->focusedOrMainFrame();
>@@ -470,6 +490,7 @@ EditorClient::EditorClient(WebKitWebView
>     WebKitWebViewPrivate* priv = m_webView->priv;
>     g_signal_connect(priv->imContext, "commit", G_CALLBACK(imContextCommitted), this);
>     g_signal_connect(priv->imContext, "preedit-changed", G_CALLBACK(imContextPreeditChanged), this);
>+    g_signal_connect_after(m_webView, "button-press-event", G_CALLBACK(buttonPressed), this);
> }
> 
> EditorClient::~EditorClient()
Comment 12 Sergio García-Cuevas González 2009-01-28 16:09:09 PST
(In reply to comment #11)

I misplaced the braces in my latest patch.  Then I tried to correct it in an obviously wrong way.  I think I should have some rest.
Comment 13 Gustavo Noronha (kov) 2009-04-22 19:38:07 PDT
Comment on attachment 24698 [details]
Copy link locations to the primary selection

> +        WARNING: NO TEST CASES ADDED OR CHANGED

This warning should be removed. There is no behavior change for webcore; we could probably add GTK+ unit tests for this, though, in WebKit/gtk.

> +GtkClipboard* PasteboardHelperGtk::getPrimary(Frame* frame) const {
> +    WebKitWebView* webView = webkit_web_frame_get_web_view(kit(frame));
> +    return gtk_widget_get_clipboard(GTK_WIDGET (webView),
> +                                    GDK_SELECTION_PRIMARY);
> +}
> +

I believe the only problem with this patch is the incorrect placement of braces here. Whoever lands, please fix this, and rs=me on a second commit to fix the other braces in this file.

I wonder if we should review the PasteBoard files placements as a whole, sometime.
Comment 14 Jan Alonzo 2009-04-24 17:07:30 PDT
Comment on attachment 24698 [details]
Copy link locations to the primary selection

Landed in r42855. Style fixes landed in r42866.

Clearing review flag.
Comment 15 Gustavo Noronha (kov) 2009-05-06 14:54:48 PDT
Comment on attachment 27129 [details]
Preliminary primary selection paste support

> +static void buttonPressed(GtkWidget* widget, GdkEventButton* event, EditorClient* client)
> +{
> +    if (event->button == 2 && event->type == GDK_BUTTON_PRESS)
> +    {
> +        Frame* frame = core(client->m_webView)->focusController()->focusedOrMainFrame();
> +        Editor* editor = frame->editor();
> +        if (editor->canPaste())
> +        {

The patch looks right, but the if braces are bad. They should look like this:

if (a) {
    blahblah();
}

Also, we should return early, where possible, so I would change that if to:

if (!editor->canPaste())
    return;

I thought about suggesting making only 1 early-return if, but I believe we may want to use this callback for other features in the future, so it makes sense this way. r=me with those changes.
Comment 16 Gustavo Noronha (kov) 2009-05-06 16:49:04 PDT
Comment on attachment 27129 [details]
Preliminary primary selection paste support

Actually, this does not seem to work. The entries do not generate a button-press-event for the view.
Comment 17 Gustavo Noronha (kov) 2009-05-12 10:14:27 PDT

*** This bug has been marked as a duplicate of 25685 ***