Last Comment Bug 334997 - add menu bar to toolkit-based JS console in suiterunner
: add menu bar to toolkit-based JS console in suiterunner
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
-- normal (vote)
: seamonkey2.0a1
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on:
Blocks: suiterunner 335383 366901
  Show dependency treegraph
 
Reported: 2006-04-21 13:36 PDT by Robert Kaiser
Modified: 2008-07-31 04:23 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add an id to toolkit console (checked in) (1.24 KB, patch)
2006-04-21 13:40 PDT, Robert Kaiser
mconnor: review+
mconnor: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
add overlay on suite side to introduce the menubar (10.41 KB, patch)
2006-04-21 17:37 PDT, Robert Kaiser
neil: review-
neil: superreview-
Details | Diff | Splinter Review
suite overlay, v2 (8.05 KB, patch)
2006-04-23 14:35 PDT, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description User image Robert Kaiser 2006-04-21 13:36:35 PDT
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 User image Robert Kaiser 2006-04-21 13:40:55 PDT
Created attachment 219353 [details] [diff] [review]
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 User image Robert Kaiser 2006-04-21 17:37:51 PDT
Created attachment 219384 [details] [diff] [review]
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 User image Robert Kaiser 2006-04-22 03:40:12 PDT
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 User image neil@parkwaycc.co.uk 2006-04-22 14:32:02 PDT
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 User image Robert Kaiser 2006-04-23 14:35:51 PDT
Created attachment 219538 [details] [diff] [review]
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 User image neil@parkwaycc.co.uk 2006-04-24 01:56:53 PDT
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 User image Robert Kaiser 2006-04-24 04:27:18 PDT
(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 User image neil@parkwaycc.co.uk 2006-04-24 08:08:12 PDT
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 User image Robert Kaiser 2006-04-25 07:44:49 PDT
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

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