Bug 151836 - Web Inspector: add "Copy Selected" context menu item to Console
Summary: Web Inspector: add "Copy Selected" context menu item to Console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-12-03 16:41 PST by Blaze Burg
Modified: 2016-08-16 10:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2016-08-13 23:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (3.28 KB, patch)
2016-08-14 13:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (3.88 KB, patch)
2016-08-15 22:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Blaze Burg 2015-12-03 16:41:25 PST
Copying item(s) from console is not discoverable enough.
Comment 1 Radar WebKit Bug Importer 2015-12-03 16:41:36 PST
<rdar://problem/23752839>
Comment 2 Devin Rousso 2016-08-13 23:42:58 PDT
Created attachment 286021 [details]
Patch
Comment 3 Nikita Vasilyev 2016-08-14 12:00:31 PDT
Comment on attachment 286021 [details]
Patch

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

This patch allows to copy one console message. That's a good start. Ideally, I'd expect the context menu to do the same as Command-C — copy all selected messages.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:344
> +

Why is this needed?

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:358
> +            contextMenu.appendItem(WebInspector.UIString("Copy Selected"), () => {

You should add "Copy Selected" to localizedStrings.js.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:-365
> -

Why is this needed?
Comment 4 Devin Rousso 2016-08-14 13:07:18 PDT
Comment on attachment 286021 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:344
>> +
> 
> Why is this needed?

Since a context menu is considered a "mousedown" but not a "mouseup" we need to force this event

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:-365
>> -
> 
> Why is this needed?

This prevented the right click from even working on console entries
Comment 5 Devin Rousso 2016-08-14 13:08:00 PDT
Created attachment 286026 [details]
Patch
Comment 6 Joseph Pecoraro 2016-08-15 17:15:15 PDT
Comment on attachment 286026 [details]
Patch

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

r=me, but this should add some comments to the tricky bit.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:344
> +        if (!this._selectedMessages.length)
> +            this._mouseup(event);

This really needs a comment. It is not obvious why this needs to be here and it is not explained anywhere. What are the consequences of not doing this? Selection handling is not reset so just moving the mouse would select rows?
Comment 7 Devin Rousso 2016-08-15 22:02:41 PDT
Created attachment 286150 [details]
Patch
Comment 8 WebKit Commit Bot 2016-08-16 10:07:30 PDT
Comment on attachment 286150 [details]
Patch

Clearing flags on attachment: 286150

Committed r204511: <http://trac.webkit.org/changeset/204511>
Comment 9 WebKit Commit Bot 2016-08-16 10:07:34 PDT
All reviewed patches have been landed.  Closing bug.