add menu bar to toolkit-based JS console in suiterunner

RESOLVED FIXED in seamonkey2.0a1

Status

SeaMonkey
UI Design
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

Trunk
seamonkey2.0a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
No longer depends on: 334478
(Assignee)

Comment 1

11 years ago
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.
Attachment #219353 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #219353 - Flags: review? → review?(mconnor)

Updated

11 years ago
Attachment #219353 - Flags: review?(mconnor)
Attachment #219353 - Flags: review+
Attachment #219353 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 2

11 years ago
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.
Attachment #219384 - Flags: superreview?(neil)
Attachment #219384 - Flags: review?(neil)
(Assignee)

Comment 3

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

Cross-comitted to trunk and 1.8 branch.
Attachment #219353 - Attachment description: add an id to toolkit console → add an id to toolkit console (checked in)

Comment 4

11 years ago
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?
Attachment #219384 - Flags: superreview?(neil)
Attachment #219384 - Flags: superreview-
Attachment #219384 - Flags: review?(neil)
Attachment #219384 - Flags: review-
(Assignee)

Comment 5

11 years ago
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 :)
Attachment #219384 - Attachment is obsolete: true
Attachment #219538 - Flags: superreview?(neil)
Attachment #219538 - Flags: review?(neil)

Comment 6

11 years ago
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)
Attachment #219538 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 7

11 years ago
(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.

Updated

11 years ago
Attachment #219538 - Flags: review?(neil) → review+

Comment 8

11 years ago
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.
(Assignee)

Updated

11 years ago
Blocks: 335383
(Assignee)

Comment 9

11 years ago
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
No longer blocks: 335383
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Blocks: 335383
(Assignee)

Updated

11 years ago
Blocks: 328887
(Assignee)

Updated

10 years ago
Blocks: 366901

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.