Bring back Error Console to Seamonkey after removal in Bug 1278368.

RESOLVED FIXED in seamonkey2.47

Status

SeaMonkey
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: frg, Assigned: frg, NeedInfo)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.47
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.43 unaffected, seamonkey2.44 unaffected, seamonkey2.45 unaffected, seamonkey2.46 unaffected, seamonkey2.47 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Bug 1278368 removed the error console. make installer is currently broken because of it. 

We need to either do this too or fork the code over to suite. I am for forking if feasible. The new one has much more functionality but is clumsy to use if you only want to check things out quick.

Comment 1

2 years ago
+1 for forking. So, the idea is to keep the old error console in parallel to the new devtools?
Similar to the DOM Inspector? (though that one is an extension anyway...)
(Assignee)

Comment 2

2 years ago
>> So, the idea is to keep the old error console in parallel to the new devtools?

That would be my plan. Like it is now in 2.45 and 2.46.

FRG
(Assignee)

Comment 3

2 years ago
I have started with a fork. Needs some tinkering before a patch is ready. If the decision later is to remove it I will just use it in my private tree :)

FRG

Updated

2 years ago
Blocks: 1284287

Updated

2 years ago
Blocks: 1284288
(Assignee)

Comment 4

2 years ago
Created attachment 8768146 [details] [diff] [review]
1282286-ForkConsole-WIP.patch

WIP patch. Works with classic theme. Will likely not work with modern theme. Testing this next.

Paths might not be optimal. 

Took out an ifdef in custom.css for windows. Would need a preprocessor step otherwise and already has a -moz-os-version: windows-xp in it so I think it's not needed.

Questions: Where should the icons reside finally. Chrome locations ok?
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8768146 - Flags: feedback?(philip.chee)
Attachment #8768146 - Flags: feedback?(iann_bugzilla)
(Assignee)

Comment 5

2 years ago
Oh I forgot. Should we keep the Ctrl+Shift+J access key as it is taken by the new console?

Comment 6

2 years ago
Bug reports against the old Error Console were recently closed because Firefox's new Browser Console supposedly does not have those problems.  With the forking to retain Error Console in SeaMonkey, will those bug reports be reopened?  More important, will any of them be fixed?

Comment 7

2 years ago
My guess is we'd have to reopen/refile them as SeaMonkey bugs.
(Assignee)

Comment 8

2 years ago
I am against just reopening and assigning them to Seamonkey. When I look at old Seamonkey bugs I see a lot which might no longer be valid. Lets first port it and then new ones can be filed if necessary in followp bugs.
(Assignee)

Comment 9

2 years ago
Btw. The devtools browser console will be there too so if you need new functionality please file a bug against it. I just want the old one around for quick checks if something is wrong. The fancy stuff should go in the new console.

FRG

Comment 10

2 years ago
Agreed to both points.
Comment on attachment 8768146 [details] [diff] [review]
1282286-ForkConsole-WIP.patch

Review of attachment 8768146 [details] [diff] [review]:
-----------------------------------------------------------------

shouldn't there be entries in the suite/installer/package-manifests.in?

i.e.
@RESPATH@/components/jsconsole-clhandler.manifest
@RESPATH@/components/jsconsole-clhandler.js

or something like that?
(Assignee)

Comment 12

2 years ago
Created attachment 8769418 [details] [diff] [review]
1282286-interimfix.patch

>> shouldn't there be entries in the suite/installer/package-manifests.in?

They are still there. The patch doesn't have them because they were currently not removed. 

But that also means that make installer will fail right now. How about an interim patch until the console is ready for action again?
Attachment #8769418 - Flags: review?(ewong)
(Assignee)

Comment 13

2 years ago
Created attachment 8769446 [details] [diff] [review]
1282286-errorconsole.patch

This one works. Set windowtype to suite:console from global:console and changed -jsconsole command line parameter to -oldconsole so that it does not interfere with the Borwser console (which did over ride this starting with 2.45 anyway).

Need to test on Linux and Windows XP. Anyone able to do and test an osx build?
Attachment #8768146 - Attachment is obsolete: true
Attachment #8768146 - Flags: feedback?(philip.chee)
Attachment #8768146 - Flags: feedback?(iann_bugzilla)
Attachment #8769446 - Flags: review?(philip.chee)
Attachment #8769446 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 14

2 years ago
Created attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

Minor fix for XP icons. Override for console-toolbar.png was not ok.

Tested with Linux and seems to be ok there.
Attachment #8769446 - Attachment is obsolete: true
Attachment #8769446 - Flags: review?(philip.chee)
Attachment #8769446 - Flags: review?(iann_bugzilla)
Attachment #8769449 - Flags: review?(philip.chee)
Attachment #8769449 - Flags: review?(iann_bugzilla)

Updated

2 years ago
Attachment #8769418 - Flags: review?(ewong) → review+
(Assignee)

Comment 15

2 years ago
Installer temporary fix

https://hg.mozilla.org/comm-central/rev/2a96f2cb7620

I will put it back into the main patch once I got some feedback on this one.
(In reply to Frank-Rainer Grahl from comment #15)
> Installer temporary fix
> 
> https://hg.mozilla.org/comm-central/rev/2a96f2cb7620
> 
> I will put it back into the main patch once I got some feedback on this one.

Why not ifdef?

Comment 17

2 years ago
(In reply to Frank-Rainer Grahl from comment #15)
> Installer temporary fix
> 
> https://hg.mozilla.org/comm-central/rev/2a96f2cb7620
> 
> I will put it back into the main patch once I got some feedback on this one.

I have used that since June 29. ;)
(Assignee)

Comment 18

2 years ago
>> Why not ifdef?

Only two lines which can be put back easily.

FRG

Comment 19

2 years ago
Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

This looks good to me and certainly works, but I would like to have history imported from the original files. I think Ratty may have done it before but there is also a few guides available on how to do it.
f+ for the moment.
Attachment #8769449 - Flags: feedback+

Updated

2 years ago
Depends on: 1288981

Comment 20

2 years ago
Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

Now the pain of file copy/history is over, this patch can be rebased. Thanks.
Attachment #8769449 - Flags: review?(philip.chee)
Attachment #8769449 - Flags: review?(iann_bugzilla)

Comment 22

2 years ago
Comment on attachment 8769449 [details] [diff] [review]
1282286-errorconsole-V2.patch

I did flatten the structure so that console/content/ -> console/ but if you prefer to have a content/ folder then that is not an issue.
>+++ b/suite/common/console/jsconsole-clhandler.js
>@@ -0,0 +1,40 @@
>+/* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */
>+/* vim:sw=4:sr:sta:et:sts: */
Nit: This line is incomplete, my editor complains that */ is an unknown option for sts

>+    if (!cmdLine.handleFlag("oldconsole", false))
Maybe errorconsole instead of oldconsole?

>+      wwatch.openWindow(null, "chrome://communicator/content/console/console.xul", "_blank",
Nit: bring "_blank", onto the following line.
>+                        "chrome,dialog=no,all", cmdLine);

>+  helpInfo : "  --oldconsole        Open the Error console.\n",
Maybe errorconsole instead of oldconsole?

>+++ b/suite/common/console/jsconsole-clhandler.manifest
>+category command-line-handler t-jsconsole @mozilla.org/suite/console-clh;1
Should this be changed from "t-jsconsole" to something like "s-errorconsole"?

>+++ b/suite/common/console/tests/.eslintrc
This didn't make into the file copy, do we actually want it?

>+++ b/suite/common/console/tests/chrome.ini
>@@ -0,0 +1,4 @@
>+[DEFAULT]
>+skip-if = buildapp == 'b2g'
Does this make any sense to us?

>+++ b/suite/common/console/tests/test_hugeURIs.xul
>@@ -0,0 +1,64 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
global or communicator?
Have you tested this test?

>+++ b/suite/common/jar.mn
>-% overlay chrome://global/content/console.xul chrome://communicator/content/consoleOverlay.xul
>+% overlay chrome://communicator/content/console/console.xul chrome://communicator/content/consoleOverlay.xul
Could we not merge consoleOverlay.xul into console.xul now? (and associated files) - maybe in a follow-up patch so we can get this landed quickly.

> % overlay chrome://help/content/help.xul chrome://communicator/content/helpOverlay.xul
Similarly for this in helpviewer?
(Assignee)

Comment 23

2 years ago
Created attachment 8775128 [details] [diff] [review]
1282286-errorconsole-V3.patch

>> I did flatten the structure so that console/content/ -> console/ but if you 

I am fine with it.

>> Maybe errorconsole instead of oldconsole?

I choose option 3 and named it suiteconsole. errorconsole is used in m-c for the devtools console. But the window and menu still say 'Error Console' so you decide if suiteconsole is ok or just use errorconsole.

>> Should this be changed from "t-jsconsole" to something like "s-errorconsole"?
I left it as it is. Changing the manifest file was educated guesswork anyway and it corresponds to the source name this way:) 

>> >+++ b/suite/common/console/tests/.eslintrc
>> This didn't make into the file copy, do we actually want it?

I removed the tests dir. The fix made it a long time ago into the tree and I doubt we will make any further changes to the console unless its broken.

>> Could we not merge consoleOverlay.xul into console.xul now?

I will do a follow-up patch.
Attachment #8769449 - Attachment is obsolete: true
Attachment #8775128 - Flags: review?(iann_bugzilla)
(Assignee)

Updated

2 years ago
Summary: Port changes from Bug 1278368 to Seamonkey. Error Console has been replaced by Browser Console. → Bring back Error Console to Seamonkey after removal in Bug 1278368.

Comment 24

2 years ago
Comment on attachment 8775128 [details] [diff] [review]
1282286-errorconsole-V3.patch

>+++ b/suite/common/console/console.css
>@@ -1,66 +1,66 @@
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
Nit: Extra blank line.
> 
> .console-box {

>+++ b/suite/common/console/console.xul
>-<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> 
>-<?xml-stylesheet href="chrome://global/skin/console/console.css" type="text/css"?> 
>-<?xml-stylesheet href="chrome://global/content/console.css" type="text/css"?> 
>+<?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
>+<?xml-stylesheet href="chrome://communicator/content/console/console.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://communicator/skin/console/console.css" type="text/css"?>
Why the switch in order?

>+++ b/suite/common/console/consoleBindings.xml
>@@ -1,24 +1,24 @@
> <?xml version="1.0"?>
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> 
Nit: Extra blank line.
> 
>-<!DOCTYPE bindings SYSTEM "chrome://global/locale/console.dtd">

>+++ b/suite/themes/classic/jar.mn

>+#ifdef XP_WIN
>+% override chrome://communicator/skin/console/console-toolbar.png   chrome://communicator/skin/console/console-toolbar-XP.png      osversion<6
>+#endif
>\ No newline at end of file
Nit: can this be fixed whilst you're here?

r=me with those addressed.
Attachment #8775128 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 25

2 years ago
Created attachment 8775155 [details] [diff] [review]
1282286-errorconsole-V3a.patch

Lates round of nits taken care of.

Review+ from IanN carried forward.
Attachment #8775128 - Attachment is obsolete: true
Attachment #8775155 - Flags: review+
(Assignee)

Comment 27

2 years ago
Could someone test next mac nightly build if its ok. If not please reopen.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.47: affected → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1289794
(Assignee)

Updated

2 years ago
Blocks: 1289981

Updated

2 years ago
Duplicate of this bug: 1289981

Comment 29

2 years ago
Fixed for 2.47
Target Milestone: --- → seamonkey2.47

Updated

2 years ago
Flags: needinfo?(philip.chee)

Updated

2 years ago
Depends on: 1296955
You need to log in before you can comment on or make changes to this bug.