Bug 16144 – [GTK] Clipboard/ selection handling functions

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Christian Dywan]

It is currently not possible to handle clipboard or selection related actions such as copy, paste or retrieving selected text.

[Christian Dywan]

Created attachment 17534 [details]
Implement several editor functions

This patch implements several editor functions, three of them are unimplemented.

[Nigel Tao]

I'm not knowledgable enough to review the meat of this patch, but in WebKit/gtk/Api/webkitgtkpage.h you've declared webkit_page_can_cut_clipboard twice.

[Luca Bruno]

Hi,
this issue has been fixed for the clipboard part in #15911.

[Christian Dywan]

Created attachment 17743 [details]
Update api, use new action signals

This is an updated version, taking the new action signals into account.

[Alp Toker]

These don't look quite like the GtkTextView way of doing things, but I didn't investigate deeply. Can you give an API rationale for this set of changes?

[Luca Bruno]

Most of them are in GtkEditable interface.
We should wait for this: http://bugs.webkit.org/show_bug.cgi?id=16286

[Alp Toker]

Another concern is that the equivalent functions in GtkTextBuffer accept GtkClipboard parameters.

This patch is looking like it will need more thought.

[Alp Toker]

GtkEditable turns out not to be worth implementing. See http://bugs.webkit.org/show_bug.cgi?id=16286 for details.

On this bug, I'm still waiting for comments on whether we can make the API look more similar to other GTK+ widgets, or whether we're stuck with using the text command API and its limitations.

[Luca Bruno]

We shouldn't emit the signal when _*_clipboard is called. Instead _real_copy must call _copy_clipboard with a default GtkClipboard*.
We have to create a WebCoreSupport bridge between platform and the API so that Pasteboard can emit a signal on the WebView.

[Xan Lopez]

(In reply to comment #4)
> Created an attachment (id=17743) [edit]
> Update api, use new action signals
> 
> This is an updated version, taking the new action signals into account.
> 

I was halfway re-implementing this when alp pointed me to this bug, so a few comments:

+        return frameData->frame->editor()->canCopy() || frameData->frame->editor()->canCopy();

missing the DTHML bit in the second one?

+        g_signal_emit_by_name(webView, "cut-clipboard");

Can we emit the signals with g_signal_emit? We are storing the identifiers after all.

That's it, actually. Of course most functions are missing the docs, but I guess you know that :)

I've checked the GtkClipboard stuff with Alp and I'd say we don't need an explicit parameter for it at all. We'll double check with Owen, but I'd say using GDK_SELECTION_CLIPBOARD internally should be good enough?

[Luca Bruno]

I think this is not the right way. First of all, i can't see why we need to emit the signal. Just call _copy_clipboard from _real_copy_clipboard. If you see, gtktextview/buffer don't emit the signal trough API.
Finally, also gtktextbuffer allows passing GtkClipboard instead GtkEditable doesn't have it.
I think we have to talk with WebCore guys to discuss about passing extradata when calling editing commands such as cut/copy/paste and be careful with this API.

[Xan Lopez]

(In reply to comment #11)
> I think this is not the right way. First of all, i can't see why we need to
> emit the signal. Just call _copy_clipboard from _real_copy_clipboard. If you
> see, gtktextview/buffer don't emit the signal trough API.

You need to emit the signal because there might be people connected to it. Those widgets don't have equivalent functions to these, that's why they don't emit the signal anywhere.

> Finally, also gtktextbuffer allows passing GtkClipboard instead GtkEditable
> doesn't have it.
> I think we have to talk with WebCore guys to discuss about passing extradata
> when calling editing commands such as cut/copy/paste and be careful with this
> API.
> 

[Luca Bruno]

> You need to emit the signal because there might be people connected to it.
> Those widgets don't have equivalent functions to these, that's why they don't
> emit the signal anywhere.

Why? The could have the equivalent functions for emitting the signal as well, just they don't.

[Christian Dywan]

Created attachment 17961 [details]
Updated accprding to comments

Removed surplus "if (frameData->frame)" checks Holger pointed out.

Using g_signal_emit as suggested by Xan.

Use performDelete() instead of clear().

Added missing documentation.

[Luca Bruno]

I think you should get the focused frame instead:

Frame* frame = core(webView)->focusController()->focusedOrMainFrame();

And remove all those old lines of code.

[Christian Dywan]

Created attachment 18006 [details]
Updated to use focusedOrMainFrame()

[Xan Lopez]

(In reply to comment #16)
> Created an attachment (id=18006) [edit]
> Updated to use focusedOrMainFrame()
> 

This looks good to me. Seems we alredy handle the clipboard stuff for GDK_SELECTION_CLIPBOARD in PasteboardGtk.cpp (what about selected text and GDK_SELECTION_PRIMARY?), so I'd say let's commit this.

[Alp Toker]

Comment on attachment 18006 [details]
Updated to use focusedOrMainFrame()

r=me

ChangeLog entry next time please. There are a few spelling mistakes and the gchar* return needs to remain private as discussed until string memory management is dealt with.

[Alp Toker]

Landed in r28942 with discussed changes.

[Xan Lopez]

Created attachment 18056 [details]
0001-Fix-signal-ids-in-g_signal_emit-for-clipboard-functi.patch

Subject: [PATCH] Fix signal ids in g_signal_emit for clipboard functions.

---
 WebKit/gtk/ChangeLog                 |   11 +++++++++++
 WebKit/gtk/WebView/webkitwebview.cpp |    8 ++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

[Alp Toker]

Comment on attachment 18056 [details]
0001-Fix-signal-ids-in-g_signal_emit-for-clipboard-functi.patch

r=me

[Alp Toker]

Follow-up patch landed in r28958.