Bug 52974 – [Qt] Add CopyImageUrlToClipboard action to QWebPage and QWKPage

• Component: WebKit Qt
• Status: RESOLVED FIXED

[szyk]

enum QWebPage::WebAction doesn't have value QWebPage::CopyImageUrl but obviously this is very frequent  action performed by users. By now I workaround this gap, but I think that such notorious action should by supported by default in standard way like other actions which can be performed on the web page.

[Benjamin Poulain]

Please follow http://trac.webkit.org/wiki/QtWebKitBugs when reporing bug here.

That is an easy patch to make if you want to add it yourself to WebKit ( https://trac.webkit.org/wiki/QtWebKitContrib )

[qi]

Created attachment 80667 [details]
patch

Introduce QWebPage::CopyImageUrlToClipboard into QWebPage:WebAction.

[Andreas Kling]

Comment on attachment 80667 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80667&action=review

> Source/WebKit/qt/ChangeLog:8
> +        Add QWebPage::CopyImageUrlToClipborad into QWebPage::WebAction.

Typo, CopyImageUrlToClipboard.

> Source/WebKit/qt/Api/qwebpage.cpp:1684
> +    \value CopyImageUrlToClipboard Copy the highlighted image's url to the clipboard.

url -> URL

> Tools/ChangeLog:8
> +        Add CopyImageUrlToClipborad into QtTestbrowser.

Typo, CopyImageUrlToClipboard.

> Tools/QtTestBrowser/webview.cpp:247
> +        QAction* newTabAction = menu->addAction("Copy Image Address", webPage, SLOT(copyImageUrlToClipboard()));

The 'newTabAction' variable name doesn't make sense here.

> Tools/QtTestBrowser/webview.cpp:248
> +        menu->insertAction(menu->actions().at(4), newTabAction);

What's up with the at(4) here? How is the QMenu action list populated?

[qi]

Created attachment 80756 [details]
patch2

Based on comments to make some changes.

For the last issue "at(4)", because we already have another 3 menu items for image, they are "Open Image, Save Image, and Copy Image", and I want to put "Copy Image Address" after them, that' why. By the way, the other 3 actions come from WebKit common code, currently "Copy Image Address" is only for Qt, so we have to manually add into ContextMenu like "Open in Default Browser" did.

[Benjamin Poulain]

Food for thought: should that be added to the default actions of the context menu?

[Chang Shu]

> For the last issue "at(4)", because we already have another 3 menu items for image, they are "Open Image, Save Image, and Copy Image", and I want to put "Copy Image Address" after them, that' why. By the way, the other 3 actions come from WebKit common code, currently "Copy Image Address" is only for Qt, so we have to manually add into ContextMenu like "Open in Default Browser" did.

It's always good to avoid hardcoded numbers. Can't we do anything like append or get the length of current items, etc.?

[qi]

Let's make it more clear, currently, the ContextMenu like:

Open Image
Save Image
Copy Image
---------- (seperator)
Inspector


I want to insert the new menu item after "Copy Image", because I think it should be group with "Open...". So, you see it is difficult to use "append" or "length" to find the position. :(


Or, like Benjamin said put it into default actions. But I think then it will go into all the platform. Some platform, like Safari already have this, I think they suppose to add it manually like what I am doing now. If I put it into default, it maybe broke their behavior.

[Chang Shu]

good explanation. :)

[Benjamin Poulain]

(In reply to comment #7)
> Or, like Benjamin said put it into default actions. But I think then it will go into all the platform. Some platform, like Safari already have this, I think they suppose to add it manually like what I am doing now. If I put it into default, it maybe broke their behavior.

Someone already changed the behavior of the menu for this release. There is now "select all" for input fields.
If we change the menu, it is best to do it all at once and communicate about the changes (on the Qt blog for example).

I have CCed Andrea, the lead of the Rekonq project. Maybe he will have an opinion on the question.

The default menu is bad enough that hybrid and mobile redefine it anyway ;)

[Benjamin Poulain]

Comment on attachment 80756 [details]
patch2

You should not have to create the action yourself like you do in QTestBrowser.
QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser.

[Benjamin Poulain]

(In reply to comment #10)
> (From update of attachment 80756 [details])
> You should not have to create the action yourself like you do in QTestBrowser.
> QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser.

And an autotest would be nice... :)

[qi]

(In reply to comment #10)
> (From update of attachment 80756 [details])
> You should not have to create the action yourself like you do in QTestBrowser.
> QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser.

I believe QWebPage::action() only for customize the display string. 

What items should be display by default is decided by WebCore/page/ContextMenuController.cpp/populate(), if we put this action into default (send it into common code), then we don't need manually add it into menu.

[Benjamin Poulain]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 80756 [details] [details])
> > You should not have to create the action yourself like you do in QTestBrowser.
> > QWebPage::action() should give you the action. You forgot to implement that part, that is why you need that code in QtTestBrowser.
> 
> I believe QWebPage::action() only for customize the display string. 

No it is not, it is the way standard actions are created. As a user, you should not have to do the connect from the action to actionTriggered.

Please check the code.....

[qi]

Created attachment 80794 [details]
patch3

Make some change based on comments, but I don't know how to create a auto test case (#11), do you have any example for it? Benjamin.

[Andreas Kling]

Comment on attachment 80794 [details]
patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=80794&action=review

> Source/WebKit/qt/Api/qwebpage.cpp:2740
> +        case CopyImageUrlToClipboard:
> +            text =  tr("Copy Image Address");
> +            break;

This should be done through the platform strategy.
We'll need a contextMenuItemTagCopyImageURLToClipboard(), though it can be #if PLATFORM(QT) specific.

[qi]

Created attachment 81059 [details]
patch4

Put this action into standard action but Qt specific.

[Early Warning System Bot]

Attachment 81059 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7693321

[Andreas Kling]

Comment on attachment 81059 [details]
patch4

Like EWS says, this breaks WebKit2. And speaking of WebKit2, the CopyImageUrlToClipboard action should also be added to the QWKPage WebActions.

[qi]

Created attachment 81090 [details]
patch5

Weird, I didn't get any build error in my workspace. Anyway, I changed webkit2 code and make a clean build, it works fine.

[Early Warning System Bot]

Attachment 81090 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7691541

[qi]

Created attachment 81211 [details]
patch6

fix webkit2 build issue.

[Andreas Kling]

Comment on attachment 81211 [details]
patch6

r=me

[WebKit Commit Bot]

Comment on attachment 81211 [details]
patch6

Clearing flags on attachment: 81211

Committed r77892: <http://trac.webkit.org/changeset/77892>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.