146075 - 'Message > Edit Message As New' (== 'Edit As New...') set an incorrect initial "X-Priority:" value (when present), which messes up 'Options > Priority' U.I. also; and code cleanup.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[varada]

Choose a mail with "high" priority, then select "Edit Message As New", select
Options | Priority, it always shows "Normal", then send the mail. It will send
with "high" priority.

To work around, select Options | Priority, then select "Normal" even it shows
"Normal" from the screen.  Another word, need to re-select the option priority
before sending the mail.

Comment 1

[Michael Buckland]

Discussed in mail news bug meeting.  Decided to minus this bug.

Comment 2

[(not reading, please use [e-mail] instead)]

taking all of varada's bugs.

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4a) Gecko/20030401]

Confirming Description (on W95).

Testcase:
1. Compose and send a message with P=High:
   UI "High" and msg "X-Priority: 2 (high)": both correct.
2. AMAN the sent message, and send it with no modification:
   UI "Normal" and msg "X-Priority: 2 (2 (high))": both incorrect !
3. AMAN the sent message, and send it with P=High (reselect it):
   UI "High" and msg "X-Priority: 2 (high)": both correct.

Comments:
1 and 3: using the Priority UI behaves (= re-set) as intended.
2: it's A.M.A.N. only which sets a wrong Priority initially.
   [Rewriting Summary accordingly.]

Workaround:
 *Allways set Priority manually once.
  [Changing Severity from Normal to Minor.]

The fix should be straight forward.

Comment 4

[Serge Gautherie (:sgautherie)]

Following my comment 3:

Here is my guessing(!) after looking with LXR:

The priority from the initial message is retrieved at line
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompUtils.cpp#341
I assume that the value of 'pPriority' is "2 (high)".

Then, the new priority header line is built at lines
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompUtils.cpp#656
"X-Priority: "
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompUtils.cpp#663
"X-Priority: 2 ("
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompUtils.cpp#669
"X-Priority: 2 (2 (high)"
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsMsgCompUtils.cpp#670
"X-Priority: 2 (2 (high))"

I believe that this code misses to handle its own (initial) format.
I don't know where the UI part is handled and how it is affected by this.

(It's up to you now.)

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.4b) Gecko/20030507]

Bug still there. (with v1.3 profile, at least)

NB: "Save As > Draft" can be used instead of actually sending the new message :-)


***** A) ("MsMail" case)

Another case, like comment 4 (= Mozilla case):

Initial/Received message contains:
[
X-Priority: 1
X-MSMail-Priority: High
]
I assume that "X-MSMail-Priority" is ignored by Mozilla (Messenger & Compose).

New/Composed (and sent) message behaves as:
"1"
becomes
"X-Priority: "
"X-Priority: 1"
"X-Priority: 1)"


1) Compose does not "enforce" a defined format, like "number (text)" for example.
2) Furthermore, it generates "incorrect" values :-(

Since Messenger upper right pane Priority column seems to _always_ display the
right (text) value, its source code may be a clue on how to change the current
code to fix point 1 directly !?

I guess the key here is to brake the current one step (string based) rebuild
code, into a 2 steps (meaning based) code.

The setting code from the Menu 'Options > Priority > ...' may be another clue.


***** B) Is a capital character needed ?

example:
In Messenger upper right pane, the Priority is displayed as "High": fine with me.
But Compose generates "high": I'd like this to be fixed. (trivial)

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) 'mime_generate_headers()'

This patch should fix most, if not all, of this bug. (I hope)
I did a little cleanup/optimization too.

NS_MsgGetPriorityFromString(...):
*Comments addition/removal
*[retPriority] replaced by [*outPriority]
*Make use of [nsMsgPriority::Default]

mime_generate_headers(...)
*[RRT_HEADER:] removal
*Enforce [X-Priority: <pValue> (<pName>)] output format  <-- Bug FIX !!

There is a few "on purpose" white space reformatting too, localized where I
updated the code.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 136141 [details] [diff] [review]
(Av1) 'mime_generate_headers()'

(see comment 6)

Can you review, compile, test, check it in ?

Comment 8

[Serge Gautherie (:sgautherie)]

Addition to comment 6:

Consequently, the <pName> will now get a capitalized initial :-)

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <FilterEditor.dtd> [Checked in: comment 21]

(It's not really this bug, but)
This patch makes the FilterEditor U.I. consistent with the "back-end":
by using capitalized initials.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 136178 [details] [diff] [review]
(Bv1) <FilterEditor.dtd>
[Checked in: comment 21]


(see comment 9)

Can you review, test, check it in ?

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <MsgComposeCommands.js> [Checked in: Comment 37]

(I'm not sure if/how it relates to this bug,
it could be separate like '<FilterEditor.dtd> patch v1',
or actually "needed" by 'mime_generate_headers()' patch v1',
but)
This patch should make things consistent with the "back-end":
by using capitalized initials.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 136181 [details] [diff] [review]
(Cv1) <MsgComposeCommands.js>
[Checked in: Comment 37]

(see comment 11)

Can you review, test, check it in ?

Comment 13

[[e-mail]]

Comment on attachment 136141 [details] [diff] [review]
(Av1) 'mime_generate_headers()'

Trying someone who should understand this stuff...

Comment 14

[[e-mail]]

Comment on attachment 136178 [details] [diff] [review]
(Bv1) <FilterEditor.dtd>
[Checked in: comment 21]

Textual change to make the case on the filter's "Change the priority to"
drop-down match that of its "Priority" "is" drop-down.

Comment 15

[David :Bienvenu]

Comment on attachment 136141 [details] [diff] [review]
(Av1) 'mime_generate_headers()'

this looks OK to me, but I haven't tried compiling or running it.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 136141 [details] [diff] [review]
(Av1) 'mime_generate_headers()'


Obsoleted because
{
+      PUSH_STRING(ltoa(priorityValue));
}
would get 6->2 instead of 1->5 :-(

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) 'mime_generate_headers()'

This is ['mime_generate_headers()' patch v1],
plus:

*Additional/full reformatting, narrowed on modified functions.
*Additional comments.
*Optimization by replacing PL_strcasestr() by PL_strchr() on value tests.
*Re-ordering of priority tests, to be the "same" in the 3 functions, and follow
the High->Low order ;-)
 *About the value tests, tell me if you prefer the previous/"safest/slowest"
approach: values _after_ names !?
*NS_MsgGetPriorityValueString() addition !  <-- patch v1 FIX.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 136206 [details] [diff] [review]
(Av2) 'mime_generate_headers()'


(see comment 17)

Can you (super-)review, compile, test, check it in ?

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <MailNewsTypes.h> [Checked in: Comment 36]

(It's not really this bug, but)
I searched a few strings in LXR: unused, as expected !
This patch removes them.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]


(see comment 19)

Can you review, compile, check it in ?

Comment 21

[[e-mail]]

Comment on attachment 136178 [details] [diff] [review]
(Bv1) <FilterEditor.dtd>
[Checked in: comment 21]

Obsoleting checked-in patch

Comment 22

[David :Bienvenu]

Comment on attachment 136181 [details] [diff] [review]
(Cv1) <MsgComposeCommands.js>
[Checked in: Comment 37]

I've reviewed this before. Is this patch in some other bug, or has someone else
come up with the same fix?

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]


'approval1.6b=?': Trivial unused code removal.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 136181 [details] [diff] [review]
(Cv1) <MsgComposeCommands.js>
[Checked in: Comment 37]


'approval1.6b=?': Trivial U.I. code cleanup.

Comment 25

[Scott MacGregor]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]

we are not going to respin 1.6b bits to pick this fix up. clearing the 1.6b
approval.

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]


'approval1.6=?': Trivial unused code removal.

Comment 27

[Serge Gautherie (:sgautherie)]

Re comment 22:

>I've reviewed this before. Is this patch in some other bug, or has someone else
>> come up with the same fix?

I'm not aware of this, but it could be;

within this bug, my guess is that you |superreview+| attachment 136178 [details] [diff] [review], 
which does the "same" change but to another file :->


*****

Adding: (CC) 'bienvenu@'

Because I keep getting (for a few weeks)
{
Hi. This is the qmail-send program at smtp.noos.fr.

<[e-mail]>:
Connected to 208.186.46.10 but connection died. (#4.4.2)
I'm not going to try again; this message has been in the queue too long.
}
(What's wrong ? Only this email address does this to me.)

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]

please request approval *after* you've secured the necessary reviews

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Av2b) 'mime_generate_headers()'

Av2, enhanced, and with trunk update.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 137956 [details] [diff] [review]
(Av2b) 'mime_generate_headers()'


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 31

[Serge Gautherie (:sgautherie)]

Retargeting from v1.1a to v1.7a !

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Av2c) 'mime_generate_headers()'

Av2b, with leftover '*' removed.

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 137972 [details] [diff] [review]
(Av2c) 'mime_generate_headers()'


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 34

[(not reading, please use [e-mail] instead)]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]

sr=sspitzer

Comment 35

[(not reading, please use [e-mail] instead)]

Comment on attachment 136181 [details] [diff] [review]
(Cv1) <MsgComposeCommands.js>
[Checked in: Comment 37]

sr=sspitzer

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 136207 [details] [diff] [review]
(Dv1) <MailNewsTypes.h>
[Checked in: Comment 36]


Check in: { 01/11/2004 06:54	neil%parkwaycc.co.uk	mozilla/ mailnews/
public/ MailNewsTypes.h      1.13 }

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 136181 [details] [diff] [review]
(Cv1) <MsgComposeCommands.js>
[Checked in: Comment 37]


Check in: { 01/11/2004 06:58	neil%parkwaycc.co.uk	mozilla/ mailnews/
compose/ resources/ content/ MsgComposeCommands.js   1.340 }

Comment 38

[Frederic Bezies]

Seems to bust composition window in thunderbird built with latest sources
up-to-date at midnight mozilla.org today.

Bug was reported in this message :

http://forums.mozillazine.org/viewtopic.php?p=329496#329496

Comment 39

[Serge Gautherie (:sgautherie)]

Re comment 38:

Copied from <http://forums.mozillazine.org/viewtopic.php?p=329496#329496>:
{
BaldGuy:
I知 using Mozilla Thunderbird 0.5a (20040113)
When I click Write or New message I get an error.
An error occurred while creating a message compose window. Please try again.
I always use the latest from MozJF with no problem but this one is broken.

MozJF:
Looks like backporting bugfix for bug 146075 killed composition window :[
Well you have to go back to 10th january build.
}

0) Can anyone else confirm ?

1) Which patch is involved ? (Cv1 and Dv1 were both checked in on 2004.01.11)

2) Could someone with ThunderBird knowledge have a look at its sources to find
out what is going wrong ? (the two patches are so trivial...)

Comment 40

[Matthias Bockelkamp]

Comment on attachment 137972 [details] [diff] [review]
(Av2c) 'mime_generate_headers()'

Build error with mingw!

c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp: In function `nsresult
   NS_MsgGetPriorityValueString(int, nsString&)':
c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp:210: error: no match for
   'operator!' in '!outValueString'
c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp:210: error: candidates
   are: operator!(bool) <built-in>
c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp: In function `nsresult
   NS_MsgGetUntranslatedPriorityName(int, nsString&)':
c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp:246: error: no match for
   'operator!' in '!outName'
c:/Mozilla/mozilla/mailnews/base/util/nsMsgUtils.cpp:246: error: candidates
   are: operator!(bool) <built-in>

Comment 41

[timeless]

in at least some instances the null check is attached to the wrong param

+nsresult NS_MsgGetPriorityFromString(
+  const char *priority,
+  nsMsgPriorityValue &outPriority)
 {
+  if (!outPriority)
+    return NS_ERROR_NULL_POINTER;

it should check priority.

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
> (From update of attachment 137972 [details] [diff] [review])
> Build error with mingw!

(In reply to comment #41)
> in at least some instances the null check is attached to the wrong param

I'm checking all these: thanks, stay tuned.

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Av2d) 'mime_generate_headers()'

Av2c, with comment 40 suggestion(s),
including comment 41 suggestion(s), and an added |const|,
plus related file (previously missed) consequences.

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Comment 44

[Matthias Bockelkamp]

Comment on attachment 149973 [details] [diff] [review]
(Av2d) 'mime_generate_headers()'

Build error with mingw!

c:/Mozilla/mozilla/mailnews/compose/src/nsMsgCompUtils.cpp: In function `char*
   mime_generate_headers(nsMsgCompFields*, const char*, int, nsIPrompt*,
   PRInt32*)':
c:/Mozilla/mozilla/mailnews/compose/src/nsMsgCompUtils.cpp:710: error: cannot
   convert `nsAutoString' to `const char*' for argument `2' to `char*
   PL_strcpy(char*, const char*)'
c:/Mozilla/mozilla/mailnews/compose/src/nsMsgCompUtils.cpp:710: error: cannot
   convert `nsAutoString' to `const char*' for argument `1' to `PRUint32
   PL_strlen(const char*)'
c:/Mozilla/mozilla/mailnews/compose/src/nsMsgCompUtils.cpp:712: error: cannot
   convert `nsAutoString' to `const char*' for argument `2' to `char*
   PL_strcpy(char*, const char*)'
c:/Mozilla/mozilla/mailnews/compose/src/nsMsgCompUtils.cpp:712: error: cannot
   convert `nsAutoString' to `const char*' for argument `1' to `PRUint32
   PL_strlen(const char*)'

Comment 45

[Serge Gautherie (:sgautherie)]

Attachment: (Av2e) 'mime_generate_headers()'

Av2d, with comment 44 suggestion(s),
plus:
*Changing from |nsAutoString| to |nsCAutoString|, and related |cStr| removal;
*Changing from |.Append*()| / |.Assign()| to |+=| / |=|;
*Removing useless |!= NULL|;
*Changing from |nsString &| to |nsACString &|, and related
NS_LITERAL_(C)STRING.

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
(Obviously, I'm learning ns*String here...)

Comment 46

[Matthias Bockelkamp]

Comment on attachment 150049 [details] [diff] [review]
(Av2e) 'mime_generate_headers()'

It compiles now. The problem with step two (showing priority normal when EMAN)
still occurs but the generated headers look all the same ("X-Priority: 2
(High)").

I also got some warnings which you might want to fix additionally:

nsMsgFilterList.cpp
Building deps for nsMsgFilterList.cpp
/cygdrive/c/Mozilla/mozilla/build/cygwin-wrapper g++ -mno-cygwin -o
nsMsgFilterList.o -c -DOSTYPE=\"WINNT5.1\" -DOSARCH=\"WINNT\" 
-I../../../../dist/include/xpcom -I../../../../dist/include/xpcom_obsolete
-I../../../../dist/include/string -I../../../../dist/include/mailnews
-I../../../../dist/include/msgdb
c:/Mozilla/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp: In member
   function `nsMsgFilterList::nsMsgFilterList()':
c:/Mozilla/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp:71: warning:
unused
   variable `nsresult rv'
c:/Mozilla/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp: In member
   function `virtual nsresult nsMsgFilterList::ClearLog()':
c:/Mozilla/mozilla/mailnews/base/search/src/nsMsgFilterList.cpp:159: warning:
unused
   variable `nsresult rv'

Comment 47

[Serge Gautherie (:sgautherie)]

Attachment: (Av2f) 'mime_generate_headers()'

Av2e, with comment 46 suggestion(s).

Then, this fixes the "back-end" only:
Well, I'll look further into the UI part,
which will probably lead to an additional (part E) patch...

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
(compile: 2 warning removals;
 test: check the 3 |nsIMsgMdnGenerator::e*Type| cases)

Comment 48

[Serge Gautherie (:sgautherie)]

Comment on attachment 150073 [details] [diff] [review]
(Av2f) 'mime_generate_headers()'


David, you already sr+ Av1;
I'm asking again for this much improved patch.

Comment 49

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) 'Options > Priority > ...' menu

Eventually, this is the fix to the initial bug report :-)

*Back-port, and extend, |onpopupshowing="updatePriorityMenu();"| from
ThunderBird :-|
 *Add |value|.
 *--> UI: Reverse priority order: Lowest->Highest to Highest->Lowest. <--
  (Highest on top, and Lowest at bottom, feels more intuitive !)
*A few JS cleanup.
*Disable |Priority| menu if helpless. (= merge duplicated code/tests too)
 *--> helpwanted: Where could I move that code so it would be called only once
per "message/window", instead of at every |Options| menu opening ? <--
*Rewrite |PriorityMenuSelect(target)|
 *Remove now useless |id|.
 *Include/GetRidOf |"priotity_highest"| spell mistake fix :->
 *Prevent "dataloss" when the only changed data is the priority. (most useful
in EditAsNew usecase)
*Return early if same priority selected. (most useful, now, when the only
changed data is the priority)

EditAsNew usecase only:
This UI code relies on the fact that the back-end code sets a valid initial
priority value, if any.
This is not the case with v1.7rc2 code, which sets priority to whatever is
written in the message body:
even (part A) "<pValue> (<pName>)" format doesn't match the current UI
"<pName>" format !
This case makes Mozilla crash !!
--> Then, this fix must not be checked-in before fixing the back-end: <--
which I'll now look into...

I'll file a new bug to forward-port this patch to TB, after check-in.

Comment 50

[Serge Gautherie (:sgautherie)]

Comment on attachment 150091 [details] [diff] [review]
(Ev1) 'Options > Priority > ...' menu


'(s)r=?': (see comment 49)
There is a UI (value order) change to acknowledge,
and a UI (disable behaviour) change to acknowledge,
and a code flow optimization which I need a hint for.
Thanks.

Comment 51

[Serge Gautherie (:sgautherie)]

(In reply to comment #49)
> Created an attachment (id=150091)
> (Ev1) 'Options > Priority > ...' menu
> 
> *Return early if same priority selected. (most useful, now, when the only
> changed data is the priority)

I meant "when selecting the same P. value", = no change !

Comment 52

[Serge Gautherie (:sgautherie)]

(In reply to comment #49)
> Created an attachment (id=150091)
> (Ev1) 'Options > Priority > ...' menu
> 
> EditAsNew usecase only:
> This UI code relies on the fact that the back-end code sets a valid initial
> priority value, if any.
> This is not the case with v1.7rc2 code, which sets priority to whatever is
> written in the message body:
> even (part A) "<pValue> (<pName>)" format doesn't match the current UI
> "<pName>" format !
> This case makes Mozilla crash !!
> --> Then, this fix must not be checked-in before fixing the back-end: <--
> which I'll now look into...

I tried, but I loose track in |nsMailboxService::PrepareMessageUrl()|.

helpwanted:
Can anyone point me to where the "Template" message is actually parsed to fill
the |var msgCompFields = gMsgCompose.compFields;| (.priority) value ?

Comment 53

[Matthias Bockelkamp]

Comment on attachment 150073 [details] [diff] [review]
(Av2f) 'mime_generate_headers()'

It comiles without warnings and errors now. But how must I test this? Every
time I insert a "Disposition-Notification-To:" or"Return-Receipt-To:" header,
Mozilla removes it before sending... Is there more info available on this (I
cannot find it in this bug's comments)?

Comment 54

[Serge Gautherie (:sgautherie)]

(In reply to comment #53)
> (From update of attachment 150073 [details] [diff] [review])
> But how must I test this? Every
> time I insert a "Disposition-Notification-To:" or"Return-Receipt-To:" header,
> Mozilla removes it before sending...

I can get |Disposition-Notification-To:| by checking 'Options > Return Receipt':
this is only added when actually sending the message, not when Saving it as Draft.

But I don't even know how to ask for the 2 other cases: missing UI !?
How did you insert these headers ?

> Is there more info available on this (I cannot find it in this bug's comments)?

No, I improved the code when I saw it...

Comment 55

[Serge Gautherie (:sgautherie)]

(In reply to comment #54)
> But I don't even know how to ask for the 2 other cases: missing UI !?
> How did you insert these headers ?

These may be related to bug 93085 comment 15 !!?

Comment 56

[Serge Gautherie (:sgautherie)]

Neil (or anyone):
Could you help me with these questions:

Part E:
Where can I move the |updatePriorityMenu()| call,
so it would execute only once par new message display, instead of at each
priority menu opening ?

Part A, 'EditAsNew' case:
Where is the Priority message line actually read/parsed/stored ?

Thanks.

Comment 57

[Serge Gautherie (:sgautherie)]

Comment on attachment 150091 [details] [diff] [review]
(Ev1) 'Options > Priority > ...' menu


(For the record)

{{
+  if (selectedPriority == msgPriority)
}}
should be |selectedPriority.value| to be useful.

I'll post an updated patch, with this and more, later.

Comment 58

[[e-mail]]

(In reply to comment #56)
>Where can I move the |updatePriorityMenu()| call,
>so it would execute only once per new message display, instead of at each
>priority menu opening ?
There is an advantage of executing it at each menu opening; an opening menu
knows to automatically reset the checked values on the other menuitems. But if
you only want to do it once, do it in ComposeStartup near FillIdentitylist.

Comment 59

[Matthias Bockelkamp]

Comment on attachment 150073 [details] [diff] [review]
(Av2f) 'mime_generate_headers()'

For the eDntType case:

Mozilla generates the correct header and removes it when replying. If Return
Receipt is checked when replying, the header is ok too.

But I still don't have any idea how to test the other cases. At first I tried
to edit my drafts file, but, as i wrote, that did'nt work. Perhaps make Bug
93085 a dependency of this one?

Comment 60

[Frank Wein [:mcsmurf]]

*** Bug 275392 has been marked as a duplicate of this bug. ***

Comment 61

[taomyn]

This isn't as minor as is lead to believe (and I'm amazed how long this bug has
been present). Mail servers that look for the X-Priority values are failing
because headers come through looking like (from Thunderbird):

X-Priority: 1 (1 (Highest))

X-Priority: 1 (1 (1 (Highest)))

X-Priority: 1 (1 (1 (1 (Highest))))

etc etc with each iteration of "Edit as new" usage, so unless every SMTP server
is sub-string matching then their checks will never detect the correct priority.

Comment 62

[Serge Gautherie (:sgautherie)]

Attachment: (Av2g) 'mime_generate_headers()'

Av2f, updated to current tree: a few string handling changes...

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

(I hope that I'll get someone(s) to r+sr it this time...)

Comment 63

[Matthias Bockelkamp]

It compiles without warnings. X-Priority and Disposition-Notification-To headers
work as they should. Still no idea how to test the Return-Receipt-To header.

Comment 64

[Serge Gautherie (:sgautherie)]

Comment on attachment 169485 [details] [diff] [review]
(Av2g) 'mime_generate_headers()'

Neil: (or someone else)
Could you review this patch,
at least the ns*String class and method uses, which I'm pretty sure can be
improved over what I managed to do. (helpwanted)

Comment 65

[Wayne Mery (:wsmwk)]

Question - did restoration of X-whatever headers *ever* work? Or, did something break it?  MDN Bug 144296 for example.


(In reply to comment #64)
> (From update of attachment 169485 [details] [diff] [review] [edit])
> Neil:  Could you review this patch ...

Serge, does patch need to be updated given the passage of time without review?


Matthias in comment #63:
>X-Priority and Disposition-Notification-To headers work as they should. Still no idea how to test the Return-Receipt-To header.

Should this patch then solve bugs for drafts and templates, for example:
 Bug 169171
 Bug 216479


Serge, your patch for related Bug 131190 wasn't checked in.

Bug 205900 mentions a patch in bug 260482, for "In-Reply-To header; similar 
support for X-Priority"

Comment 66

[Serge Gautherie (:sgautherie)]

(In reply to comment #65)
> Question - did restoration of X-whatever headers *ever* work? Or, did something
> break it?  MDN Bug 144296 for example.

I'm not sure what you're asking;
but I believe this bug is not a regression (== that it never worked).

> (In reply to comment #64)
> > (From update of attachment 169485 [details] [diff] [review] [edit] [edit])
> > Neil:  Could you review this patch ...
> 
> Serge, does patch need to be updated given the passage of time without review?

I'll update it in a moment.

> Matthias in comment #63:
> >X-Priority and Disposition-Notification-To headers work as they should. Still no idea how to test the Return-Receipt-To header.
> 
> Should this patch then solve bugs for drafts and templates, for example:
>  Bug 169171
>  Bug 216479

I added new dependencies there.
My A patch should solve the 'Priority' case(s),
but only that one (data).

> Serge, your patch for related Bug 131190 wasn't checked in.

Not my patch actually, I was passing by there only.

> Bug 205900 mentions a patch in bug 260482, for "In-Reply-To header; similar 
> support for X-Priority"

Right, but that is a different issue (than this bug).

Comment 67

[Serge Gautherie (:sgautherie)]

Attachment: (Av2h) 'mime_generate_headers()' [Checkin: Comment 73 & 78]

Av2g, updated to current tree. (un-compiled)

[
Neil: (or someone else)
Could you review this patch,
at least the ns*String class and method uses, which I'm pretty sure can be
improved over what I managed to do. (helpwanted)
]
still applies, even more after all this time.

Comment 68

[[e-mail]]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]

Will try this out later but I'm not sure what I'm looking for.

>+  // Note: Checking the values separately and _before_ the names,
>+  //        hoping for a much faster match;
>+  //       Only _drawback_, as "priority" handling is not truly specified:
>+  //        some softwares may have the number meanings reversed (1=Lowest) !?
>+  if (PL_strchr(priority, '1'))
>+    outPriority = nsMsgPriority::highest;
>+  else if (PL_strchr(priority, '2'))
>+    outPriority = nsMsgPriority::high;
>+  else if (PL_strchr(priority, '3'))
>+    outPriority = nsMsgPriority::normal;
>+  else if (PL_strchr(priority, '4'))
>+    outPriority = nsMsgPriority::low;
>+  else if (PL_strchr(priority, '5'))
>+    outPriority = nsMsgPriority::lowest;
I'm wondering here whether the digit is always the first character.

>+      NS_ASSERTION(PR_FALSE, "invalid priority value");
I think there's a better way of writing this, something like NS_NOTREACHED.

The only way I could think of improving the string handling is for those methods that return one of six strings to return a const char * (one of six string literals) rather than assigning to an out parameter.

Comment 69

[Serge Gautherie (:sgautherie)]

(In reply to comment #68)
> (From update of attachment 241431 [details] [diff] [review] [edit])
> Will try this out later but I'm not sure what I'm looking for.

Feature-wise ?
The text format [X-Priority: <pValue> (<pName>)] should be enforced.
As a side-effect, the GUI menu should be initialized with the proper value.

String-wise ?
I'm not familiar with using or not ns*String, .AssignWithConversion(), .AssignLiteral(), ...: so I thought I might not have done it as right/best as it should be.

> >+  // Note: Checking the values separately and _before_ the names,
> >+  //        hoping for a much faster match;
> >+  //       Only _drawback_, as "priority" handling is not truly specified:
> >+  //        some softwares may have the number meanings reversed (1=Lowest) !?
> >+  if (PL_strchr(priority, '1'))
> {...}
> I'm wondering here whether the digit is always the first character.

As far as I know, it is (if present).

I thought I had found some kind of related documentation at that time, but I don't remember where.
What I find now is the "2.1.3.3.1. Table 3: Priority Mappings" at <http://tools.ietf.org/html/rfc4356>.
(This mentions the "Importance:" property too, but I leave it aside for some later followup, if needed.)

> >+      NS_ASSERTION(PR_FALSE, "invalid priority value");
> I think there's a better way of writing this, something like NS_NOTREACHED.
> 
> The only way I could think of improving the string handling is for those
> methods that return one of six strings to return a const char * (one of six
> string literals) rather than assigning to an out parameter.

Yes, I could do both in a later patch :-)

Comment 70

[[e-mail]]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]

This doesn't actually set the UI up correctly but it seems to do everything else.

Comment 71

[Serge Gautherie (:sgautherie)]

helpwanted: I don't have a build environment for the time being.

Comment 72

[[e-mail]]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]

Assuming it's OK to fix the back end and UI in separate patches...

Comment 73

[Serge Gautherie (:sgautherie)]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]


Checkin: {
2006-10-10 02:28	neil%parkwaycc.co.uk
}

Comment 74

[[e-mail]]

Attachment: SeaMonkey port of UI Fix [Checkin: See comment 91]

Differences between this and the Thunderbird code:
1. Doesn't reset the menu between messages as opening it resets it anyway
2. Passes the menupopup directly into updatePriorityMenu
3. Doesn't bother unchecking the old menuitem, menus do that automatically
4. Moves the onpopupshowing attribute to the menupopup
5. Removes the unnecessary checked attribute in the XUL
6. Whitespace fix for the lowest priority menuitem

Comment 75

[Serge Gautherie (:sgautherie)]

(In reply to comment #74)
> Created an attachment (id=242047) [edit]
> SeaMonkey port of UI Fix

Ah ! More than 2.5 years ago was to much for my memory :-/

This reminds me of my [(Ev1) 'Options > Priority > ...' menu] patch.
And this is why I was wrong lately: my [(Av2h) 'mime_generate_headers()'] was backend only, as you (re)discovered.

Now, rereading comment 49 (and comment 58), I think your patch could enhanced...

NB:
Could my Av2h patch be a? and a+ (already) ?
That would help me work (again) on this, as I can use v1.8 branch builds only.

Comment 76

[[e-mail]]

OK, so let's see how our patches differ:
>*A few JS cleanup.
>*Disable |Priority| menu if helpless. (= merge duplicated code/tests too)
I didn't bother with these.
>*--> helpwanted: Where could I move that code so it would be called only once
per "message/window", instead of at every |Options| menu opening ? <--
This could be done, but then some people never use that menu.
>*Remove now useless |id|.
>*Prevent "dataloss" when the only changed data is the priority. (most useful
in EditAsNew usecase)
Good points... Mnyromyr, should I do these?

Comment 77

[[e-mail]]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]

(In reply to comment #75)
>Could my Av2h patch be a? and a+ (already) ?
approval-thunderbird2 seems to be the most relevant flag.

Comment 78

[Serge Gautherie (:sgautherie)]

Comment on attachment 241431 [details] [diff] [review]
(Av2h) 'mime_generate_headers()'
[Checkin: Comment 73 & 78]


Checkin: {
2006-10-18 15:18	neil%parkwaycc.co.uk 	MOZILLA_1_8_BRANCH
}

Comment 79

[Karsten Düsterloh]

Comment on attachment 242047 [details] [diff] [review]
SeaMonkey port of UI Fix
[Checkin: See comment 91]

>Index: MsgComposeCommands.js
>===================================================================
>+function updatePriorityMenu(priorityMenu)
>+{
>+  if (gMsgCompose)
>+  {
>+    var msgCompFields = gMsgCompose.compFields;
>+    if (msgCompFields && msgCompFields.priority)
>+      priorityMenu.getElementsByAttribute( "value", msgCompFields.priority )[0].setAttribute('checked', 'true');
>+  }
>+}

This simply doesn't work: priority gets assigned the exact string value from the mail header which could be _anything_ - and even for regular X-Priority headers (like we create ourself) this won't work, because eg. priority == '4 (Low)', not 'Low' as in the menuitem values.

Thus, opening the priority menu will give

Error: priorityMenu.getElementsByAttribute("value", msgCompFields.priority)[0] has no properties
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2025

Especially with recycled compose windows, this will lead to false selected values in the priority menu!

> function PriorityMenuSelect(target)
...
>+      msgCompFields.priority = target.getAttribute('value');

This, otoh, _does_ work, since the values like 'Low' are recognized by the backend (see nsMsgUtils.cpp).

>Index: messengercompose.xul
>===================================================================
>+              <menuitem type="radio" name="priority" label="&highestPriorityCmd.label;" accesskey="&highestPriorityCmd.accesskey;" value="Highest" id="priority_highest"/>
>+              <menuitem type="radio" name="priority" label="&highPriorityCmd.label;" accesskey="&highPriorityCmd.accesskey;" value="High" id="priority_high"/>
>+              <menuitem type="radio" name="priority" label="&normalPriorityCmd.label;" accesskey="&normalPriorityCmd.accesskey;" value="Normal" id="priority_normal"/>
>+              <menuitem type="radio" name="priority" label="&lowPriorityCmd.label;" accesskey="&lowPriorityCmd.accesskey;" value="Low" id="priority_low"/>
>+              <menuitem type="radio" name="priority" label="&lowestPriorityCmd.label;" accesskey="&lowestPriorityCmd.accesskey;" value="Lowest" id="priority_lowest"/>

Yeas, drop the ids if you are not using them anymore.

Comment 80

[[e-mail]]

Attachment: Fix for comment #79 [Checkin: Comment 83]

Comment 81

[Scott MacGregor]

Comment on attachment 247186 [details] [diff] [review]
Fix for comment #79
[Checkin: Comment 83]

Nice work Neil! this seems like a good candidate for the branch.

Comment 82

[[e-mail]]

Comment on attachment 242047 [details] [diff] [review]
SeaMonkey port of UI Fix
[Checkin: See comment 91]

Rerequesting review as per comment #81.

Comment 83

[Serge Gautherie (:sgautherie)]

Comment on attachment 247186 [details] [diff] [review]
Fix for comment #79
[Checkin: Comment 83]


Checkin: {
2006-12-01 12:12	neil%parkwaycc.co.uk 	mozilla/mailnews/mime/src/mimedrft.cpp 	1.152
}

Checkin: {
2006-12-01 12:15	neil%parkwaycc.co.uk 	mozilla/mailnews/mime/src/mimedrft.cpp 	1.142.2.7 	MOZILLA_1_8_BRANCH
}

Comment 84

[Mike Cowperthwaite]

I can verify that Edit As New and reopening a Draft both restore the 
Options|Priority setting, in TB2-1202, Win2K.

Incidentally, attachment 241431 [details] [diff] [review] appears to have fixed bug 252041.


Has there been a general policy change which is encouraging marking checked-in patches as "obsolete"?  If not, why doesn't anyone stop Serge from doing so?
"Obsolete" implies "no good" -- a checked in patch is anything but.  I've complained about this before; this behavior, perhaps irrationally, really sets my teeth on edge.

Comment 85

[David :Bienvenu]

No, up until  now, afaik, only timeless does that. I don't care for it either. I think attaching the cvs checkin log is more than sufficient...but there's no policy forbidding it. As long as Serge only does it to his own patches, I can live  with it, but as you say, it's highly misleading.

Comment 86

[Serge Gautherie (:sgautherie)]

(In reply to comment #84)
> "Obsolete" implies "no good" -- a checked in patch is anything but.  I've

Well, extend it as "done with: no good anymore".

> complained about this before; this behavior, perhaps irrationally, really sets
> my teeth on edge.

I understand; yet, this bug is a good example of why I do it that way:
multiple (& parallel) patches; waiting for r/sr for multiple years; ...
This helps me to "remember/track" the status of my (remaining) patches in such situations.
(You're welcome to let me know how this could be improved...)

Comment 87

[Wayne Mery (:wsmwk)]

I don't understand why the notation "[Checkin: Comment NN]" is not sufficient - a notation BTW that I really like because in some bugs you can't tell if an approved patch has been checked in.  

isn't marking a checked in patch obsolete inconsistent with the clear understanding that bad patches are also marked obsolete, but there typically is NO notation that it is bad - it's implied.  

And then there is the case for check in patches gone bad and backed out - what is the procedure for that if the patch is already marked obsolete?

Comment 88

[Karsten Düsterloh]

Comment on attachment 247186 [details] [diff] [review]
Fix for comment #79
[Checkin: Comment 83]

Serge, please don't mark other folks' patches as obsolete just because they're checked in...

Comment 89

[Karsten Düsterloh]

Comment on attachment 242047 [details] [diff] [review]
SeaMonkey port of UI Fix
[Checkin: See comment 91]

Okay, this works now as intended... :)

>Index: MsgComposeCommands.js
>===================================================================
>+      priorityMenu.getElementsByAttribute( "value", msgCompFields.priority )[0].setAttribute('checked', 'true');

We don't use these fancy whitespace before "value" and behind .priority anywhere else, so just drop it. And maybe wrap the line behind [0] and use "" in the setAttribute, too?

>+              <menuitem type="radio" name="priority" label="&highestPriorityCmd.label;" accesskey="&highestPriorityCmd.accesskey;" value="Highest" id="priority_highest"/>

Yeah, drop the unused ids, I doubt even weird extensions would be using them.

r=me with that.

Comment 90

[[e-mail]]

Attachment: Branch version updated for review comments [Checkin: Comment 93]

Comment 91

[Serge Gautherie (:sgautherie)]

Comment on attachment 242047 [details] [diff] [review]
SeaMonkey port of UI Fix
[Checkin: See comment 91]


Checkin: {
2006-12-03 11:31	neil%parkwaycc.co.uk
}
What was actually checked in "is" attachment 247344 [details] [diff] [review].

Comment 92

[Ian Neal]

Comment on attachment 247344 [details] [diff] [review]
Branch version updated for review comments
[Checkin: Comment 93]

a-SM1.1=me (SM only)

Comment 93

[Serge Gautherie (:sgautherie)]

Comment on attachment 247344 [details] [diff] [review]
Branch version updated for review comments
[Checkin: Comment 93]


Checkin: {
2006-12-04 09:43	neil%parkwaycc.co.uk 	MOZILLA_1_8_BRANCH
}

Comment 94

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2pre) Gecko/20061229 SeaMonkey/1.1] (nightly) (W2Ksp4)

Neil, the priority menu doesn't seem to reset between messages:
Replying or composing a new message displays the previous priority.
I would expect "None" for a new message,
and "None" or "same as message"(= enhancement !?) when replying.

Comment 95

[Karsten Düsterloh]

Attachment: set a default priority value if we don't get one (SM) [Checkin: Comment 97]

If we did not get a new priority passed (ie. we're composing a new message or are replying etc.), we did not touch the priority at all, thus cached compose windows remembered the last set value...

Comment 96

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

second-a=me for sm1.1

Comment 97

[Karsten Düsterloh]

Comment on attachment 250002 [details] [diff] [review]
set a default priority value if we don't get one (SM)
[Checkin: Comment 97]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 99

[Serge Gautherie (:sgautherie)]

Comment on attachment 250002 [details] [diff] [review]
set a default priority value if we don't get one (SM)
[Checkin: Comment 97]

>-  if (gMsgCompose)
>-  {
>-    var msgCompFields = gMsgCompose.compFields;
>-    if (msgCompFields && msgCompFields.priority)
>-      priorityMenu.getElementsByAttribute("value", msgCompFields.priority)[0]
>-                  .setAttribute("checked", "true");
>-  }
>+  var priority = (gMsgCompose && gMsgCompose.compFields && gMsgCompose.compFields.priority) || "Normal";
>+  priorityMenu.getElementsByAttribute("value", priority)[0].setAttribute("checked", "true");

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2pre) Gecko/20070108 SeaMonkey/1.1] (nightly) (W2Ksp4)

Karsten, Neil:
This changes the "default" U.I. display to checked "Normal" instead of the (unchecked) legacy "None".
This seems a regression to me, as this is not in sync' with the backend anymore, and as such misleading.
The proper fix would be to simply uncheck the U.I. item if there is no priority set in the message itself.

Comment 100

[[e-mail]]

(In reply to comment #99)
>This changes the "default" U.I. display to checked "Normal" instead of the
>(unchecked) legacy "None".
Actually my 1.6 always displays "Normal" checked.

Comment 101

[Serge Gautherie (:sgautherie)]

(In reply to comment #100)
> Actually my 1.6 always displays "Normal" checked.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.9) Gecko/20061211 SeaMonkey/1.0.7] (release) (W2Ksp4)

Right: I should have double checked. Sorry :-(

I filed bug 366575 about this.

Comment 102

[Magnus Melin [:mkmelin]]

This is fixed, no?

Comment 103

[Wayne Mery (:wsmwk)]

(In reply to comment #102)
> This is fixed, no?

yes, based on testing Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081006 Shredder/3.0a3

Comment 104

[Camaleon]

This bug is hitting me right now (Thunderbird 60.6.1, 64-bits, Linux).
Maybe a regression?

Comment 105

[Wayne Mery (:wsmwk)]

(In reply to Camaleon from comment #104)

This bug is hitting me right now (Thunderbird 60.6.1, 64-bits, Linux).
Maybe a regression?

Perhaps, but certainly not a regression from a change in this bug, which was made 9 years ago.
Please start Thunderbird in safe mode and if you still see a problem file a new bug report.
Note also, Serge is no longer active.