Last Comment Bug 719196 - Port |Bug 712421 - allow pasting a URL in the download manager window to download it|
: Port |Bug 712421 - allow pasting a URL in the download manager window to down...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.9
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 13:00 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-01-20 13:42 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.15 KB, patch)
2012-01-18 13:00 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (5.30 KB, patch)
2012-01-18 14:08 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2a (4.17 KB, patch)
2012-01-19 14:37 PST, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
patch v2b [Checkin: Comment 9] (4.15 KB, patch)
2012-01-20 13:40 PST, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2012-01-18 13:00:04 PST
Created attachment 589613 [details] [diff] [review]
patch

Neil, this should sound familiar. ;-)
Comment 1 neil@parkwaycc.co.uk 2012-01-18 13:46:41 PST
Unsurprisingly you failed to read bug 712421 comment 4 though.

key_paste needs command="cmd_paste" (not sure why it doesn't get it automatically). This then needs to be handled in dlTreeController.
Comment 2 Jens Hatlak (:InvisibleSmiley) 2012-01-18 14:08:58 PST
Created attachment 589642 [details] [diff] [review]
patch v2

(In reply to neil@parkwaycc.co.uk from comment #1)
> Unsurprisingly you failed to read bug 712421 comment 4 though.

Wrong. Citing: "SeaMonkey's download manager uses a proper command controller which definitely won't conflict with the textbox" - skimming it, I read that as "there is no issue with this approach with SeaMonkey". Sure, with your explanation or looking deeper into the code it can be read differently, but it wasn't exactly crystal clear.

> key_paste needs command="cmd_paste" (not sure why it doesn't get it
> automatically). This then needs to be handled in dlTreeController.

I wasn't sure how much of the pre-checks you'll require, so please give me some advice what you'd like to see for isCommandEnabled and doCommand respectively if you don't like the single function call approach.
Comment 3 neil@parkwaycc.co.uk 2012-01-19 14:18:51 PST
Comment on attachment 589642 [details] [diff] [review]
patch v2

>       case "cmd_selectAll":
>       case "cmd_clearList":
>+      case "cmd_paste":
Nit: cmd_paste before cmd_selectAll, as it would be in editMenuCommands...

>-                "cmd_selectAll", "cmd_clearList"];
>+                "cmd_selectAll", "cmd_clearList", "cmd_paste"];
I think this change might be wrong...

>+      <command id="cmd_paste"
>+               oncommand="goDoCommand('cmd_paste');"/>
because editMenuCommands already provides cmd_paste...

>+    <key id="key_paste"
Nit: keys in edit menu order too, so after key_copy please.
[Hmm, I wonder why we don't use editMenuKeys...]
Comment 4 Jens Hatlak (:InvisibleSmiley) 2012-01-19 14:37:29 PST
Created attachment 589997 [details] [diff] [review]
patch v2a
Comment 5 neil@parkwaycc.co.uk 2012-01-19 16:09:37 PST
Comment on attachment 589997 [details] [diff] [review]
patch v2a

D'oh, I need to update my tree for bug 712421 first, of course...

>+      case "cmd_paste":
>+        handlePaste();
Bah, I forgot to ask for a break; last time...

>     <key id="key_cut"/>
>     <key id="key_copy"/>
>+    <key id="key_paste"
>+         command="cmd_paste"
>+         key="V"
>+         modifiers="accel"/>
>     <key id="key_play"
>          key=" "
>          command="cmd_play"/>
>     <key id="key_delete"/>
>     <key id="key_delete2"/>
>     <key id="key_selectAll"/>
The lack of attributes suggests we might pick up some automatically - please check and remove any you don't need.
Comment 6 neil@parkwaycc.co.uk 2012-01-20 06:18:27 PST
Comment on attachment 589997 [details] [diff] [review]
patch v2a

r=me with the above fixed.
Comment 7 Jens Hatlak (:InvisibleSmiley) 2012-01-20 13:38:40 PST
(In reply to neil@parkwaycc.co.uk from comment #5)
> >+      case "cmd_paste":
> >+        handlePaste();
> Bah, I forgot to ask for a break; last time...

Well, I made the mistake in the first place. ;-)

> >+    <key id="key_paste"
> >+         command="cmd_paste"
> >+         key="V"
> >+         modifiers="accel"/>
> The lack of attributes suggests we might pick up some automatically - please
> check and remove any you don't need.

Indeed we get "key" and "modifiers" from elsewhere, probably utilityOverlay.xul (and I guess the platformHTMLBindings.xml reference there doesn't apply to key_paste). Will remove these two.
Comment 8 Jens Hatlak (:InvisibleSmiley) 2012-01-20 13:40:15 PST
Created attachment 590325 [details] [diff] [review]
patch v2b [Checkin: Comment 9]
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-01-20 13:41:53 PST
Comment on attachment 590325 [details] [diff] [review]
patch v2b [Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/d3222f251e1c

Note You need to log in before you can comment on or make changes to this bug.