Last Comment Bug 366901 - Edit->Copy doesn't work in the SuiteRunner Error Console.
: Edit->Copy doesn't work in the SuiteRunner Error Console.
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
-- minor (vote)
: ---
Assigned To: Philip Chee
:
:
Mentors:
http://lxr.mozilla.org/seamonkey/sour...
Depends on: 334997
Blocks: suiterunner
  Show dependency treegraph
 
Reported: 2007-01-13 04:35 PST by Philip Chee
Modified: 2008-07-31 04:23 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch 1.0 (1.96 KB, patch)
2007-01-13 04:37 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch 1.1 (5.21 KB, patch)
2007-01-13 10:06 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch 1.1a (2.04 KB, patch)
2007-01-13 10:10 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch 1.1b (Checked into trunk) (2.41 KB, patch)
2007-01-14 23:27 PST, Philip Chee
neil: review+
Details | Diff | Splinter Review

Description User image Philip Chee 2007-01-13 04:35:09 PST
Bug 334997 added a menubar to the toolkit Error console when running in SeaMonkey. However the Edit->Copy menu item isn't working because the utilityOverlay pulled in overrides the toolkit error console "cmd_copy" command.

Neil on #seamonkey said to port the XPFE console's command controller. Patch coming up.
Comment 1 User image Philip Chee 2007-01-13 04:37:57 PST
Created attachment 251363 [details] [diff] [review]
Patch 1.0

Adds an inline script to consoleOverlay.xul to implement a command controller for "cmd_copy"
Comment 2 User image neil@parkwaycc.co.uk 2007-01-13 05:48:46 PST
Comment on attachment 251363 [details] [diff] [review]
Patch 1.0

>+      isCommandEnabled: function (aCommand)
>+      supportsCommand: function (aCommand) 
>+      doCommand: function (aCommand)
Nit: correct order is supportsCommand, then isCommandEnabled, then doCommand

>+      window.removeEventListener("load", initConsoleOverlay, false);
Might not have to do this in a load event.

There's still a problem in that the menuitem isn't enabling when the evaluator text box has focus, because the toolkit version of UpdateCopyMenu is different.
Comment 3 User image Philip Chee 2007-01-13 10:06:08 PST
Created attachment 251384 [details] [diff] [review]
Patch 1.1

> (From update of attachment 251363 [details] [diff] [review])
>>+      isCommandEnabled: function (aCommand)
>>+      supportsCommand: function (aCommand)
>>+      doCommand: function (aCommand)
> Nit: correct order is supportsCommand, then isCommandEnabled, then doCommand

That was the order in the XPFE console.js introduced in Bug 67647. You didn't mention anything about the order then.

> >+      window.removeEventListener("load", initConsoleOverlay, false);
> Might not have to do this in a load event.

Won't this leak when the window closes?

> There's still a problem in that the menuitem isn't enabling when the evaluator
> text box has focus, because the toolkit version of UpdateCopyMenu is different.

Is it just a matter of overriding the toolkit [u|U]pdateCopyMenu with the XPFE version? This seems to work and doesn't seem to affect the ability to copy the console items.
Comment 4 User image Philip Chee 2007-01-13 10:08:31 PST
Comment on attachment 251384 [details] [diff] [review]
Patch 1.1

Index: consoleOverlay.xul
===================================================================
RCS file: /cvsroot/mozilla/suite/common/consoleOverlay.xul,v
retrieving revision 1.1
diff -u -8 -p -r1.1 consoleOverlay.xul
--- consoleOverlay.xul	25 Apr 2006 14:33:55 -0000	1.1
+++ consoleOverlay.xul	13 Jan 2007 17:57:10 -0000
@@ -46,16 +46,72 @@
 <!ENTITY % consoleOverlayDTD SYSTEM "chrome://communicator/locale/consoleOverlay.dtd">
 %consoleOverlayDTD;
 
 ]>
 
 <overlay id="consoleOverlay"
          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
 
+  <script type="application/x-javascript"><![CDATA[
+    /* :::::::: Command Controller for the Window ::::::::::::::: */
+
+    var ConsoleController =
+    {
+      supportsCommand: function (aCommand) 
+      {
+        switch (aCommand) {
+          case "cmd_copy":
+            return true;
+          default:
+            return false;
+        }
+      },
+
+      isCommandEnabled: function (aCommand)
+      {
+        switch (aCommand) {
+          case "cmd_copy":
+            return isItemSelected();
+          default:
+            return false;
+        }
+      },
+
+      doCommand: function (aCommand)
+      {
+        switch (aCommand) {
+          case "cmd_copy":
+            copyItemToClipboard();
+            break;
+          default:
+            break;
+        }
+      },
+
+      onEvent: function (aEvent) 
+      {
+      }
+    }
+
+    function updateCopyMenu()
+    {
+      goUpdateCommand("cmd_copy");
+    }
+
+    function initConsoleOverlay()
+    {
+      window.removeEventListener("load", initConsoleOverlay, false);
+      top.controllers.insertControllerAt(0, ConsoleController);
+
+    }
+
+    window.addEventListener("load", initConsoleOverlay, false);
+  ]]></script>
+
   <commandset id="consoleCommands">
     <commandset id="tasksCommands"/>
     <command id="cmd_quit"/>
   </commandset>
 
   <keyset id="consoleKeys">
     <keyset id="tasksKeys"/>
     <key id="key_copy"/>
Comment 5 User image Philip Chee 2007-01-13 10:10:54 PST
Created attachment 251385 [details] [diff] [review]
Patch 1.1a

Whee. What fun I can't edit a patch it just becomes another comment.
Comment 6 User image neil@parkwaycc.co.uk 2007-01-14 14:20:11 PST
(In reply to comment #3)
>>Nit: correct order is supportsCommand, then isCommandEnabled, then doCommand
>That was the order in the XPFE console.js introduced in Bug 67647. You didn't
>mention anything about the order then.
Touché, but then I wasn't formally reviewing it (note the r=jag).

>>>+      window.removeEventListener("load", initConsoleOverlay, false);
>>Might not have to do this in a load event.
>Won't this leak when the window closes?
Actually I quoted the wrong line; I meant to quote the top.controllers line.
But in answer to your question, all event listeners get unhooked.
Comment 7 User image Philip Chee 2007-01-14 23:27:44 PST
Created attachment 251496 [details] [diff] [review]
Patch 1.1b (Checked into trunk)

1. Change order of members in the controller.
2. Remove the load event listener.
3. Instead of overriding the toolkit console's updateCopyMenu() we call goUpdateCommand('cmd_copy') directly in the onpopupshowing handler attribute.
Comment 8 User image neil@parkwaycc.co.uk 2007-01-15 02:27:19 PST
Comment on attachment 251496 [details] [diff] [review]
Patch 1.1b (Checked into trunk)

r+sr=me
Comment 9 User image Ian Neal 2007-01-15 07:17:36 PST
Comment on attachment 251496 [details] [diff] [review]
Patch 1.1b (Checked into trunk)

Checking in (trunk)
consoleOverlay.xul;
new revision: 1.2; previous revision: 1.1
done

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