Bug 16144 - [GTK] Clipboard/ selection handling functions
Summary: [GTK] Clipboard/ selection handling functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-11-26 12:15 PST by Christian Dywan
Modified: 2007-12-22 08:35 PST (History)
2 users (show)

See Also:


Attachments
Implement several editor functions (7.58 KB, patch)
2007-11-26 12:23 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Update api, use new action signals (5.50 KB, patch)
2007-12-06 00:50 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Updated accprding to comments (6.51 KB, patch)
2007-12-17 06:22 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Updated to use focusedOrMainFrame() (5.95 KB, patch)
2007-12-20 08:21 PST, Christian Dywan
alp: review+
Details | Formatted Diff | Diff
0001-Fix-signal-ids-in-g_signal_emit-for-clipboard-functi.patch (2.31 KB, patch)
2007-12-21 18:26 PST, Xan Lopez
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2007-11-26 12:15:20 PST
It is currently not possible to handle clipboard or selection related actions such as copy, paste or retrieving selected text.
Comment 1 Christian Dywan 2007-11-26 12:23:25 PST
Created attachment 17534 [details]
Implement several editor functions

This patch implements several editor functions, three of them are unimplemented.
Comment 2 Nigel Tao 2007-11-26 22:39:21 PST
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.
Comment 3 Luca Bruno 2007-12-04 23:41:28 PST
Hi,
this issue has been fixed for the clipboard part in #15911.
Comment 4 Christian Dywan 2007-12-06 00:50:13 PST
Created attachment 17743 [details]
Update api, use new action signals

This is an updated version, taking the new action signals into account.
Comment 5 Alp Toker 2007-12-06 12:21:50 PST
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?
Comment 6 Luca Bruno 2007-12-06 14:10:07 PST
Most of them are in GtkEditable interface.
We should wait for this: http://bugs.webkit.org/show_bug.cgi?id=16286
Comment 7 Alp Toker 2007-12-08 09:54:37 PST
Another concern is that the equivalent functions in GtkTextBuffer accept GtkClipboard parameters.

This patch is looking like it will need more thought.
Comment 8 Alp Toker 2007-12-10 07:49:48 PST
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.
Comment 9 Luca Bruno 2007-12-14 08:57:27 PST
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.
Comment 10 Xan Lopez 2007-12-15 11:20:48 PST
(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?
Comment 11 Luca Bruno 2007-12-15 15:40:14 PST
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.
Comment 12 Xan Lopez 2007-12-16 17:14:27 PST
(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.
> 

Comment 13 Luca Bruno 2007-12-17 01:35:56 PST
> 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.
Comment 14 Christian Dywan 2007-12-17 06:22:49 PST
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.
Comment 15 Luca Bruno 2007-12-17 07:02:14 PST
I think you should get the focused frame instead:

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

And remove all those old lines of code.
Comment 16 Christian Dywan 2007-12-20 08:21:52 PST
Created attachment 18006 [details]
Updated to use focusedOrMainFrame()
Comment 17 Xan Lopez 2007-12-21 10:10:53 PST
(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.
Comment 18 Alp Toker 2007-12-21 16:28:38 PST
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.
Comment 19 Alp Toker 2007-12-21 16:37:18 PST
Landed in r28942 with discussed changes.
Comment 20 Xan Lopez 2007-12-21 18:26:01 PST
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(-)
Comment 21 Alp Toker 2007-12-22 08:32:55 PST
Comment on attachment 18056 [details]
0001-Fix-signal-ids-in-g_signal_emit-for-clipboard-functi.patch

r=me
Comment 22 Alp Toker 2007-12-22 08:35:39 PST
Follow-up patch landed in r28958.