Closed
Bug 334997
Opened 19 years ago
Closed 19 years ago
add menu bar to toolkit-based JS console in suiterunner
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a1
People
(Reporter: kairo, Assigned: kairo)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
mconnor
:
review+
mconnor
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•19 years ago
|
||
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•19 years ago
|
Attachment #219353 -
Flags: review? → review?(mconnor)
Updated•19 years ago
|
Attachment #219353 -
Flags: review?(mconnor)
Attachment #219353 -
Flags: review+
Attachment #219353 -
Flags: approval-branch-1.8.1+
Assignee | ||
Comment 2•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
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•19 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•19 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•19 years ago
|
Attachment #219538 -
Flags: review?(neil) → review+
Comment 8•19 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 | ||
Comment 9•19 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
Assignee | ||
Updated•19 years ago
|
Blocks: suiterunner
You need to log in
before you can comment on or make changes to this bug.
Description
•