197781 - <startup-new*.xul>: Convert <window class="dialog"> to <dialog>

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Sebastian Späth]

In chatzilla there are two occurances of <window class="dialog"...

Converting them offers following advantages:
- there is <dialog> which is thought for dialogs (and still looks the same)
- It saves the inclusion of dialogOverlay.xul
- It saves the inclusion of the dialog <keyset> as it happens automatically
- Buttons will automatically be displayed according to chrome/OS preferences
- <OK><Cancel> buttons will not be reversed when using the "sidebar on the right
side customization" (http://mozilla.org/unix/customizing.html) as they are now

Patch coming up shortly

Comment 1

[Sebastian Späth]

Attachment: <startup-new[Script|URL].*> patch v1

When patching the files I noticed that 
/extensions/irc/xul/content/prefpanel/startup-newScript.xul seems to be
completely unused?

If that is the case and it is not planned to use it, it should be totally
removed (see http://lxr.mozilla.org/seamonkey/search?string=startup-newScript)

Otherwise looking for r=/sr= and somebody to check this in.

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 117457 [details] [diff] [review]
<startup-new[Script|URL].*> patch v1


In <startup-newScript.xul>,
remove <dialogOverlay.xul> too.

Then, you might want to ask niel@ for review...

Comment 3

[Serge Gautherie (:sgautherie)]

Sorry, I meant neil@.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: <startup-new[Script|URL].*> patch v2

Patch v1, plus comment 2 suggestions.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 136422 [details] [diff] [review]
<startup-new[Script|URL].*> patch v2


Can you review, test, check it in ?

(I'm not using ChatZilla.)

Comment 6

[[e-mail]]

Attachment: Fixed radiobutton issues [Check in: See comment 15]

I wasn't able to test this dialog properly, even without the patch.
I found one bug, which was the misuse of mRadioChildren.
So, this patch has those changes merged in.

Comment 7

[[e-mail]]

Comment on attachment 136570 [details] [diff] [review]
Fixed radiobutton issues
[Check in: See comment 15]

rginda, you'll want to fix your abuse of mRadioChildren in case bz gets his fix
for bug 226549 in.

Comment 8

[Robert Ginda]

Comment on attachment 136570 [details] [diff] [review]
Fixed radiobutton issues
[Check in: See comment 15]

this is actually james' code.

Comment 9

[James Ross]

Comment on attachment 136570 [details] [diff] [review]
Fixed radiobutton issues
[Check in: See comment 15]

Looks good, though I think startup-newScript.* aren't used any more, and could
be removed.

Comment 10

[[e-mail]]

Comment on attachment 136570 [details] [diff] [review]
Fixed radiobutton issues
[Check in: See comment 15]

Want this in because otherwise the fix for regression bug 226549 will break
this.

Comment 11

[James Ross]

Attachment: Fixes another RegExp-caused regression

Comment 12

[Serge Gautherie (:sgautherie)]

Re comment 1 (which I overlooked), and comment 9:

<http://lxr.mozilla.org/seamonkey/search?string=startup-new>

*<startup-newURL.*> is called from
{
/extensions/irc/xul/content/prefpanel/startup.js, line 121 --
window.openDialog("chrome://chatzilla/content/prefpanel/startup-newURL.xul",
/extensions/irc/xul/content/prefpanel/startup.js, line 140 --
window.openDialog("chrome://chatzilla/content/prefpanel/startup-newURL.xul",
}

*<startup-newScript.*> is unused:
neil, would you add a third patch to get rid of them ?
{
/extensions/irc/xul/locale/en-US/pref-irc.dtd, line 177 -- <!ENTITY
startup-newScript.window.title "ChatZilla Script Item">
/extensions/irc/jar.mn, line 47 --
content/chatzilla/prefpanel/startup-newScript.js
(xul/content/prefpanel/startup-newScript.js)
/extensions/irc/jar.mn, line 48 --
content/chatzilla/prefpanel/startup-newScript.xul
(xul/content/prefpanel/startup-newScript.xul)
}

Comment 13

[Serge Gautherie (:sgautherie)]

Adding 'neil@':
Please, read comment 12.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 136570 [details] [diff] [review]
Fixed radiobutton issues
[Check in: See comment 15]

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 15

[[e-mail]]

I just checked in the changes to startup-newURL.*, it seemed wasteful to check
in changes to startup-newScript.* if we're going to remove them soon.

Comment 16

[:Gijs (he/him)]

All of these changes have been checked in or applied in another way(*), and
/xul/prefpanel/* is no longer in use, as far as I'm aware.
I think this bug can and should be marked FIXED :-).

(*): 
< if (params.length >= 4 && (typeof params[3] != "undefined")) 
> if (arrayHasElementAt(params, 3))
Has the same effect.

Comment 17

[[e-mail]]

OK, but (when) are startup-newScript.* going to be CVS removed?

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 136599 [details] [diff] [review]
Fixes another RegExp-caused regression

Fixed by
{{
1.3	silver%warwickcompsoc.co.uk	2004-03-03 14:38		Bug
229072 - Fixing RegExp regression in pref panel. Patch by Ian Neal
<[e-mail]>. r=[e-mail]
}}

Comment 19

[:Gijs (he/him)]

(In reply to comment #17)
> OK, but (when) are startup-newScript.* going to be CVS removed?

I don't know. I do not have CVS access, and as far as I'm aware, at this point
Silver is/was the only person on the ChatZilla project with CVS access, and he's
cut off from the net till at least the start of April. Which means it might take
a while, unless someone from outside the project gets permission to do remove
these files from CVS and does it for us (or ssieb / rginda get their CVS access
straightened out).

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> OK, but (when) are startup-newScript.* going to be CVS removed?

<http://lxr.mozilla.org/seamonkey/search?string=startup-new>
{{
startup-new
/extensions/irc/xul/content/prefpanel/startup-newScript.xul, line 50 --
title="&startup-newScript.window.title;">
/extensions/irc/xul/content/prefpanel/startup-newScript.xul, line 53 --
src="chrome://chatzilla/content/prefpanel/startup-newScript.js"/>
/extensions/irc/xul/content/prefpanel/startup-newURL.xul, line 47 --
title="&startup-newURL.window.title;"
/extensions/irc/xul/content/prefpanel/startup-newURL.xul, line 51 -- <script
src="chrome://chatzilla/content/prefpanel/startup-newURL.js"/>
/extensions/irc/xul/content/prefpanel/startup.js, line 121 --
window.openDialog("chrome://chatzilla/content/prefpanel/startup-newURL.xul",
/extensions/irc/xul/content/prefpanel/startup.js, line 140 --
window.openDialog("chrome://chatzilla/content/prefpanel/startup-newURL.xul", 
}}

startup-newScript.*, startup-newURL.*, startup.js were all removed from
<http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/irc/jar.mn> in
{{
1.23	silver%warwickcompsoc.co.uk	2004-06-08 20:33	 	Bug 243629 - Landing of
ChatZilla 0.9.63. Lots of bugs fixed, including: 226223, 230328, 127662, 185729,
226410, 231150, 235650, 238551, 238716, 73257, 226408, 238612, 108087, 180577,
211461, 218070, 242381 and 147452. r=[e-mail]
}}
Maybe there is more than these 2 files to cvs removed !?
Samuel/silver, can you help here ?

Comment 21

[:Gijs (he/him)]

See also bug 259198. As stated in comment 16, the entire prefpanel directory
could be CVS rem'd, but whether it should / will be removed should be decided by
rginda, ssieb and/or Silver, I think.

Comment 22

[[e-mail]]

OK, we'll mark this as fixed.