334997 - add menu bar to toolkit-based JS console in suiterunner

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

In bug 334478, I'm switching suiterunner (suite-on-toolkit) to using toolkit's console, but we really want a this window to have a menu bar like the xpfe console had.
The solution is overlaying that menubar into the toolkit-based window - I'm working on a patch for that.

Comment 1

[Robert Kaiser]

Attachment: add an id to toolkit console (checked in)

For us to be able to overlay a menubar into the toolkit console, we need its <toolbox> to have an id set, which I'm adding with this patch.

Comment 2

[Robert Kaiser]

Attachment: add overlay on suite side to introduce the menubar

This is the suite side of this patch, using the id introduced by the toolkit patch to overlay the menubar onto the toolkit console.

Comment 3

[Robert Kaiser]

Comment on attachment 219353 [details] [diff] [review]
add an id to toolkit console (checked in)

Cross-comitted to trunk and 1.8 branch.

Comment 4

[[e-mail]]

Comment on attachment 219384 [details] [diff] [review]
add overlay on suite side to introduce the menubar

>-##ifdef MOZ_XUL_APP
>+#ifdef MOZ_XUL_APP
> #% content communicator %content/communicator/ xpcnativewrappers=yes
>-##else
>+% overlay chrome://global/content/console.xul chrome://communicator/content/consoleCommOverlay.xul
>+#else
> #* content/communicator/contents.rdf                        (contents.rdf)
>-##endif
>+#endif
>+  content/communicator/consoleCommOverlay.js                       (consoleCommOverlay.js)
>+  content/communicator/consoleCommOverlay.xul                      (consoleCommOverlay.xul)
Hmm... should we put this in the MOZ_XUL_APP section? (you don't need the js file because you can use goToggleToolbar from utilityOverlay.js) Also as mentioned on IRC the Comm is superfluous.

>+  <window id="JSConsoleWindow">
We use a different hack, see below ;-)

>+    <broadcaster id="Console:toggleToolbarMode" label="&toolbarMode.label;" 
>+                 oncommand="toggleToolbar(this);" checked="true" 
>+                 _toolbar="ToolbarMode"/>
>+    <broadcaster id="Console:toggleToolbarEval" label="&toolbarEval.label;"
>+                 oncommand="toggleToolbar(this);" checked="true" 
>+                 _toolbar="ToolbarEval"/>
These are pointless, really. Just put all the attributes on the menuitems.

>+    <keyset id="tasksKeys">
>+      <key id="key_copy"/>
>+      <key id="key_close"/>
>+      <key id="key_quit"/>
>+    </keyset>
>+    <commandset id="tasksCommands"/>
>+  </window>
>+  
>+  <commandset id="consoleCommands">
>+    <command id="cmd_quit"/>
>+  </commandset>
Well, you got it half right with the commandset, but you can nest sets.

>+  <toolbox id="console-toolbox">
>+    <menubar id="main-menubar" class="chromeclass-menubar" grippytooltiptext="&menuBar.tooltip;" position="1">
Nit: line exceeds 80 characters. I also spotted some trailing whitespace.

>+<!ENTITY toolbarMode.label "Mode">
>+<!ENTITY toolbarEval.label "JavaScript Entry">
Accesskeys perchance?

Comment 5

[Robert Kaiser]

Attachment: suite overlay, v2

OK, here's the second version. no trickery with overlaying the window itself needed any more, all of Neil's comments and nits addressed :)

Comment 6

[[e-mail]]

Comment on attachment 219538 [details] [diff] [review]
suite overlay, v2

>+  locale/@AB_CD@/communicator/consoleOverlay.dtd                            (%chrome/common/consoleOverlay.dtd)
>   locale/@AB_CD@/communicator/pref/pref-locales.dtd                (%chrome/common/pref/pref-locales.dtd)
Is there a reason for the different indent?

>+  <commandset id="consoleCommands">
>+    <commandset id="tasksCommands"/>
>+    <command id="cmd_quit"/>
>+  </commandset>
>+
>+  <keyset id="consoleKeys">
>+    <keyset id="tasksKeys">
>+      <key id="key_copy"/>
>+      <key id="key_close"/>
>+      <key id="key_quit"/>
>+    </keyset>
>+  </keyset>
Just list the keys inside the consoleKeys keyset, no need to nest them.

>+                        oncommand="goToggleToolbar('ToolbarMode','toggleToolbarMode');"
Nit: space after comma (both times)

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> (From update of attachment 219538 [details] [diff] [review] [edit])
> Is there a reason for the different indent?

Yes, the new file is indented so that it fits what we'll have after my pending moves of communicator chrome to suite/ - I'll reindent the other file to the same spacing either now or with those moves.

Comment 8

[[e-mail]]

Comment on attachment 219538 [details] [diff] [review]
suite overlay, v2

>+<!ENTITY menuBar.tooltip "Menu Bar">
>+<!ENTITY toolbarsCmd.label "Show/Hide">
>+<!ENTITY toolbarsCmd.accesskey "w">
>+<!ENTITY toolbarMode.label "Mode">
>+<!ENTITY toolbarMode.accesskey "M">
>+<!ENTITY toolbarEval.label "JavaScript Entry">
>+<!ENTITY toolbarEval.accesskey "J">
Oh, I meant to say it might be nice to line these up.

Comment 9

[Robert Kaiser]

Fix checked in. There's still an issue with accesskeys that Neil discovered, but we can't resolve that at the moment - either a XUL bug needs to be fixed or toolkit console changed to ease overlaying, see bug 335383