399575 - Modernize askSendFormat

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Žiga Sancin]

askSendFormat.xul doesn't use a stringbundle and has some xul workarounds, which are not needed anymore.

Comment 1

[Žiga Sancin]

Attachment: wip patch

Phil, can you check this patch?

Few notes:
1. I've replaced the whole deck with a description tag (I think it's better that way - instead of leaving empty lines if the string is shorter than the longest string inside the deck).

2. I've removed all the tags with the style attribute (argh), but the problem is with spacers that have a width or height value of 10px or 1em. There are no predefined separators with that values. Adding two thin separators looks awkard, but leaving a spacer with a style attribute is worse I guess. :) However, I could define a new separator type with a height or width value of 1em.

3. I've also modified the radiogroup, but I'm not sure if my modifications (nor the current code in the tree) are rtl aware.

Comment 2

[Žiga Sancin]

Comment on attachment 288473 [details] [diff] [review]
wip patch

Hum, it seems that Phil is busy. Magnus, do you have some time? :)

Comment 3

[Phil Ringnalda (:philor)]

Drat! Sorry about that, "forgetful" would be more accurate than "busy" - I actually have the bug open in two tabs, three now, and the patch in another.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 288473 [details] [diff] [review]
wip patch

Looks quite good, thanks for doing it! 

A few nits:
-remove the empty lines with leading spaces, you add a few of them
- trailing spaces after sendButton.setAttribute("label", askSendFormatStringBundle.getString("send"));     

- could you add accesskeys while you're at it?

Comment 5

[Žiga Sancin]

Comment on attachment 288473 [details] [diff] [review]
wip patch

Thanks for the quick review! I'll update the patch.

Comment 6

[Žiga Sancin]

Attachment: patch v2

I've added accesskeys (not sure if I chose the right accesskeys) and fixed spaces.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 291467 [details] [diff] [review]
patch v2

You could also add 
        buttonlabelaccept="Send"
        buttonaccesskeyaccept="S"
        buttonlabelcancel="Cancel"
        buttonaccesskeycancel="C"

to the asksendformat.xul dialog (with those keys moved to .dtd of course) and remove the setting of access button label in js. I'd let the Send button get the S access key.

Sorry for harassing you with accesskeys here;)

r=me with that fixed

Comment 8

[Žiga Sancin]

(In reply to comment #7)
> (From update of attachment 291467 [details] [diff] [review])
> You could also add 
>         buttonlabelaccept="Send"
>         buttonaccesskeyaccept="S"
>         buttonlabelcancel="Cancel"
>         buttonaccesskeycancel="C"
> 
> to the asksendformat.xul dialog (with those keys moved to .dtd of course) and
> remove the setting of access button label in js. I'd let the Send button get
> the S access key.
> 

Sure, I'll move the Send button label to askSendFormat.xul. Regarding the accesskeys - the XUL Accesskey FAQ [1] says that the main dialog buttons like OK, Cancel and Close don't get accesskeys. Hum?

[1] http://developer.mozilla.org/en/docs/XUL_Accesskey_FAQ_and_Policies#How_do_I_pick_an_accesskey_letter.3F

Comment 9

[Magnus Melin [:mkmelin]]

Ah, disregard my comments about the Cancel button.

Comment 10

[Žiga Sancin]

Attachment: patch v3

Done. Asking Karsten for an additional, SeaMonkey review.

Comment 11

[Karsten Düsterloh]

Comment on attachment 291884 [details] [diff] [review]
patch v3

>Index: mailnews/compose/resources/content/askSendFormat.js
>===================================================================
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2000
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Jean-Francois Ducarroz <[e-mail]>
>+ *   Ben Bucksch <[e-mail]>
>+ *   Ian Neal <[e-mail]>

Where did you get this data from? 
(Same question for askSendFormat.xul.)

>+    try
>+    {
>       defaultAction = prefs.getIntPref("mail.asksendformat.default");
>       useDefault = true;
>     } catch (ex) {}

The "catch (ex) {}" should go onto its own line.

> function Startup()
> {
>   if (window.arguments && window.arguments[0])
>-  {    
>+  {
>+    var askSendFormatStringBundle = document.getElementById("askSendFormatStringBundle");
>     param = window.arguments[0];
>-    param.abort = true;    //if the user hit the close box, we will abort.
>+    //if the user hits the close box, we will abort.

Should start with "// If".
In general, all // should be followed by a space and comments ending on ?!. should start with an uppercase letter.

>Index: mailnews/compose/resources/content/askSendFormat.xul
>===================================================================
>+  <stringbundle id="askSendFormatStringBundle" src="chrome://messenger/locale/messengercompose/askSendFormat.properties"/>

Should be wrapped, too.

>+      <separator class="thin" orient="vertical"/>
>+      <separator class="thin" orient="vertical"/>

That's not particularly nice here (and isn't the several times you used this
pattern below). It's probably better to use just one classless (hence "thick")
separator instead.

>Index: suite/locales/en-US/chrome/mailnews/compose/askSendFormat.dtd
>===================================================================
> <!ENTITY plainTextAndHtml.label "Send in Plain Text and HTML">
>+<!ENTITY plainTextAndHtml.accesskey "P">
> <!ENTITY plainTextOnly.label "Send in Plain Text Only">
>+<!ENTITY plainTextOnly.accesskey "T">
> <!ENTITY htmlOnly.label "Send in HTML Only">
>+<!ENTITY htmlOnly.accesskey "H">

I think it'd better to use 'P' for plainTextOnly, and then probably 'a' for
plainTextAndHtml.

Comment 12

[Žiga Sancin]

(In reply to comment #11)
> (From update of attachment 291884 [details] [diff] [review])
> >Index: mailnews/compose/resources/content/askSendFormat.js
> >===================================================================
> >+ * The Initial Developer of the Original Code is
> >+ * Netscape Communications Corporation.
> >+ * Portions created by the Initial Developer are Copyright (C) 2000
> >+ * the Initial Developer. All Rights Reserved.
> >+ *
> >+ * Contributor(s):
> >+ *   Jean-Francois Ducarroz <[e-mail]>
> >+ *   Ben Bucksch <[e-mail]>
> >+ *   Ian Neal <[e-mail]>
> 
> Where did you get this data from? 
> (Same question for askSendFormat.xul.)

I got it from the CVS Log. I think that those three developers made significant changes to these two files in the past, so I've added them as contributors.

Comment 13

[Žiga Sancin]

Attachment: patch v4

Nits fixed.

Comment 14

[Žiga Sancin]

Comment on attachment 293511 [details] [diff] [review]
patch v4

Asking Scott for a sr. :)

Comment 15

[Scott MacGregor]

Comment on attachment 293511 [details] [diff] [review]
patch v4

looks good bisi!

Comment 16

[Magnus Melin [:mkmelin]]

Checking in mail/locales/jar.mn;
/cvsroot/mozilla/mail/locales/jar.mn,v  <--  jar.mn
new revision: 1.52; previous revision: 1.51
done
Checking in mail/locales/en-US/chrome/messenger/messengercompose/askSendFormat.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/askSendFormat.dtd,v  <--  askSendFormat.dtd
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/askSendFormat.properties,v
done
Checking in mail/locales/en-US/chrome/messenger/messengercompose/askSendFormat.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messengercompose/askSendFormat.properties,v  <--  askSendFormat.properties
initial revision: 1.1
done
Checking in mail/themes/qute/mail/dialogs.css;
/cvsroot/mozilla/mail/themes/qute/mail/dialogs.css,v  <--  dialogs.css
new revision: 1.3; previous revision: 1.2
done
Checking in mailnews/compose/resources/content/askSendFormat.js;
/cvsroot/mozilla/mailnews/compose/resources/content/askSendFormat.js,v  <--  askSendFormat.js
new revision: 1.22; previous revision: 1.21
done
Checking in mailnews/compose/resources/content/askSendFormat.xul;
/cvsroot/mozilla/mailnews/compose/resources/content/askSendFormat.xul,v  <--  askSendFormat.xul
new revision: 1.24; previous revision: 1.23
done
Checking in suite/locales/jar.mn;
/cvsroot/mozilla/suite/locales/jar.mn,v  <--  jar.mn
new revision: 1.27; previous revision: 1.26
done
Checking in suite/locales/en-US/chrome/mailnews/compose/askSendFormat.dtd;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/askSendFormat.dtd,v  <--  askSendFormat.dtd
new revision: 1.10; previous revision: 1.9
done
RCS file: /cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/askSendFormat.properties,v
done
Checking in suite/locales/en-US/chrome/mailnews/compose/askSendFormat.properties;
/cvsroot/mozilla/suite/locales/en-US/chrome/mailnews/compose/askSendFormat.properties,v  <--  askSendFormat.properties
initial revision: 1.1
done
Checking in suite/themes/classic/messenger/dialogs.css;
/cvsroot/mozilla/suite/themes/classic/messenger/dialogs.css,v  <--  dialogs.css
new revision: 1.4; previous revision: 1.3
done
Checking in suite/themes/modern/messenger/dialogs.css;
/cvsroot/mozilla/suite/themes/modern/messenger/dialogs.css,v  <--  dialogs.css
new revision: 1.5; previous revision: 1.4
done

->FIXED

Looking forward to more patches, bisi;)

Comment 17

[Žiga Sancin]

Thanks Magnus! :)