Closed Bug 1289794 Opened 3 years ago Closed 3 years ago

Integrate consoleoverlay files

Categories

(SeaMonkey :: General, enhancement)

enhancement
Not set

Tracking

(seamonkey2.48 fixed)

RESOLVED FIXED
seamonkey2.48
Tracking Status
seamonkey2.48 --- fixed

People

(Reporter: frg, Assigned: frg, NeedInfo)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1282286 moved the old error console to suite. 

The files suite\common\consoleOverlay.xul and suite\locales\en-US\chrome\common\consoleOverlay.dtd could now be integrated into the new code and then be removed.
Version: unspecified → Trunk
Flags: needinfo?(philip.chee)
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Summary: Remove consoleoverlay from Seamonkey → Integrate consoleoverlay files
Attached patch 1289794-consoleoverlay.patch (obsolete) — Splinter Review
The xul got quite a few new items so I completely reformatted it.
Attachment #8778623 - Flags: review?(iann_bugzilla)
Comment on attachment 8778623 [details] [diff] [review]
1289794-consoleoverlay.patch

>+++ b/suite/common/console/console.xul
>   <commandset id="consoleCommands">
>-    <command id="cmd_close" oncommand="closeWindow(true)"/>
>+    <command id="cmd_close"
>+             oncommand="closeWindow(true)"/>
Nit: This change is not needed, if all attributes can fit in a single line (less than 80 characters), it is acceptable formatting.

>+    <commandset id="tasksCommands"/>
I'd prefer this at the top of the parent <commandset>

>+    <key id="key_focus1"
>+         key="&focus1.commandkey;"
>+         modifiers="accel"
>          oncommand="gTextBoxEval.focus()"/>
If you're changing the lines around it, you might as well put a ; after the )

>+    <key id="key_focus2"
>+         key="&focus2.commandkey;"
>+         modifiers="alt"
>          oncommand="gTextBoxEval.focus()"/>
If you're changing the lines around it, you might as well put a ; after the )

>+    <keyset id="tasksKeys"/>
I'd prefer this at the top of the parent <keyset>

>-      <menuitem id="menu_copy_cm" command="cmd_copy"
>-                label="&copyCmd.label;" accesskey="&copyCmd.accesskey;"/>
>+      <menuitem id="menu_copy_cm"
>+                command="cmd_copy"
>+                label="&copyCmd.label;"
>+                accesskey="&copyCmd.accesskey;"/>
Preference is to have id, label then accesskey which leaves, in this case, command being the last attribute.

>+    <menubar id="main-menubar"
>+             class="chromeclass-menubar"
>+             grippytooltiptext="&menuBar.tooltip;"
>+             position="1">
I think position is only needed in overlay files.

>+              <menuitem id="toggleToolbarMode"
>+                        type="checkbox"
>+                        checked="true"
>+                        oncommand="goToggleToolbar('ToolbarMode','toggleToolbarMode');"
>+                        label="&toolbarMode.label;"
>+                        accesskey="&toolbarMode.accesskey;"/>
Nit: have oncommand as the last attribute.
>+              <menuitem id="toggleToolbarEval"
>+                        type="checkbox"
>+                        checked="true"
>+                        oncommand="goToggleToolbar('ToolbarEval','toggleToolbarEval');"
>+                        label="&toolbarEval.label;"
>+                        accesskey="&toolbarEval.accesskey;"/>
Nit: have oncommand as the last attribute.

>+      <toolbarbutton id="Console:clear"
>+                     oncommand="clearConsole();"
>+                     label="&clear.label;"
>+                     accesskey="&clear.accesskey;"/>
Nit: have oncommand as the last attribute.

>+      <textbox id="TextboxEval"
>+               class="toolbar"
>+               value=""
>+               onkeypress="onEvalKeyPress(event)"
>+               flex="1"/>
Nit: have onkeypress as the last attribute and ; after ). flex can be after class.

>+      <toolbarbutton id="ButtonEval"
>+                     label="&evaluate.label;"
>+                     accesskey="&evaluate.accesskey;"
>+                     oncommand="evaluateTypein()"/>
Nit: have ; after ).

>+    <statusbarpanel flex="1"
>+                    pack="start">
Nit: This change is not needed, if all attributes can fit in a single line (less than 80 characters), it is acceptable formatting.

>+      <label value="&filter2.label;"
>+             control="Filter"/>
Nit: This change is not needed, if all attributes can fit in a single line (less than 80 characters), it is acceptable formatting.

>+      <textbox accesskey="&filter2.accesskey;"
>+               type="search"
>+               id="Filter"
>+               oncommand="changeFilter();"/>
Nit: accesskey after id please

r/a=me for those addressed.
Attachment #8778623 - Flags: review?(iann_bugzilla) → review+
Patch with nits fixed. Review+ from IanN carried forward.
Attachment #8778623 - Attachment is obsolete: true
Attachment #8787868 - Flags: review+
https://hg.mozilla.org/comm-central/rev/5831664662c8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.48
You need to log in before you can comment on or make changes to this bug.