141531 - move quote reply pref (reply below/above) to account settings

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[Dan Howard]

When replying to a message, Mozilla allows the user to set a preference of
quoting the original message and starting the reply either above or below the
quote.  This preference is currently universal (i.e., a single preference is
applied to all accounts).

The Mozilla newsgroup guidelines recommend bottom-posting and including selected
text, which is logical in the newsgroup context.  Email users appear to prefer
top-posting.  This forces the Mozilla user bypass the set preference in either
email or newsgroup posting by moving the cursor to the beginning or end of the
quoted text, contrary to the set preference.

Replying to messages would be simplified by allowing the user to select separate
options for email and newsgoups posting.  The most direct way to accomplish this
would appear to be moving the preference selection to the account settings page.
 In the alternative, it could involve two boxes in the Preferences/Composition
page, or separate pages for the two functions.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This should be over in the mailnews-specific prefs dialog anyway....

Comment 2

[Nicolás Lichtmaier]

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

Comment 3

[Jean-Francois Ducarroz]

-->varada

Comment 4

[Alfonso Martinez]

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

Comment 5

[Peter Lairo]

Even better would be a global _and_ an account-specific setting (very much like
for "Addressing / LDAP" setings).

-------------------------------------------------------------------------------
GLOBAL:  Reply [ below ] message.

-------------------------------------------------------------------------------
ACCOUNT: ( ) use global settings (currently: below message)
         (o) use different setting for this account: reply [ above ] message
-------------------------------------------------------------------------------

BTW What bugs me even more, is that we don't support the way *most* business
correspondence is: Reply above _and_ THE SIGNATURE IS ALSO ABOVE THE QUOTED MAIL.

PS. some keywords (e.g., mozilla1.2) may help this get on the radar. (Dan?)

Comment 6

[Peter Lairo]

http://bugzilla.mozilla.org/show_bug.cgi?id=62429

Comment 7

[jay garcia]

Peter: Most business mail where the reply is above is that way because they use
Outlook or similar application. My company is 8,000+ strong and we demand that
replies are following the previous reply simply because chronological order
makes the most sense. Quoting above previous replies will effectively eliminate
anything following the two -- delimiter unless you manually delete it. If I
remember correctly, the default in Navigator/Communicator is reply *above*
because of a few Corporate Enterprise customers wanted it that way, it's never
been changed back to date.

Comment 8

[Peter Lairo]

[OT]
Jay: My company is 30,000+ strong and everybody (plus our clients) uses top
posting and top signature. So we can ignore reality and insist that this
standard is inflexible, or we can find a way to let people do things the way
they want.
[/OT]

Comment 9

[jay garcia]

Peter: That's fine as long as we can find a way to not further bloat the product
with an over-preferenced UI.

Comment 10

[Peter Lairo]

Jay: Preferences are good when they provide meaningful and useful choices. This
is such a case. Anyhow, it would be a preference that _can_ be left at the
defaults (mail: top-reply <-> newsgroups: bottom-reply). Currently, this most
useful scenario isn't even possible.

Currently, _every_ server pref has an *Addressing* group which _only_ contains
one LDAP setting. We could use this (nearly empty and rarely used) group, rename
it(?), and add this bug to it. It would still be a rarely used group, but users
would be able to separate mail and NG posting styles. This would be a good
improvement.

Comment 11

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

taking all of varada's bugs.

Comment 12

[Peter Lairo]

[OT] Solution to question raised in comment #9: bug 62429 comment #74 [/OT]

Comment 13

[Daniel Wang]

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

Comment 14

[jay garcia]

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

Comment 15

[Peter Lairo]

This bug should be blocking bug 62429#c70 which already has 47 votes and
countless CC's. Dependency?

Comment 16

[Ian Neal]

Looking at fixing this with patch to bug 62429 -> Taking

Comment 17

[Ian Neal]

Accepting

Comment 18

[Ian Neal]

Attachment: Preferences Composition panel v0.5b

This is my initial mockup of the preference's composition panel

Comment 19

[Ian Neal]

Attachment: Account Composition Panel v0.5b

This is my initial mockup of the Account Composition panel

Comment 20

[Peter Lairo]

Re. attachment 128902 [details] - A minor nit: Since the "composition" settings are, IMO,
more relevant than the LDAP settings, perhaps they should be moved to *above*
the LDAP settings.

Comment 21

[Ian Neal]

If they were swapped round I'd also have to retitle the pane to be "Composition
& Addressing", not a problem but the final decision would be the superreviewer's
as it is asthetics (sp?).

Comment 22

[Ian Neal]

Attachment: Initial Patch v0.5b

This patch moves the global quoting preference to a per account basis with the
UI now under Mail & Newsgroup Account settings. Probably still needs some
tidying up.

Comment 23

[Ian Neal]

Attachment: Mockup of possible UI addition

With moving quoting preference to a per account basis, we could take advantange
of that to move the compose HTML preference from the main panel to the new
"Addressing & Composition" panel. Mockup shows proposed look.

Comment 24

[Ian Neal]

Attachment: Patch v0.5c

This patch is the same as v0.5b except it additionally move the compose in HTML
option to the new "Addressing & Composition" settings pane.

Comment 25

[[e-mail]]

Comment on attachment 129017 [details] [diff] [review]
Patch v0.5c

>-[scriptable, uuid(D3B4A420-D5AC-11d2-806A-006008128C4E)]
>+[scriptable, uuid(8cc20593-88de-488e-ab85-ee38c9e4646c)]
???

>+  /* should we override global quoting preference? */
>+  attribute boolean autoQuote;
>+
>+  /* what should our local quoting preference be? */
>+  attribute long replyOnTop;
I don't think the comments match with what the attributes do.

>+  <label hidden="true" wsm_persist="true" id="identity.replyOnTop"
>+        pref="true" preftype="int" prefattribute="value"
>+        prefstring="mail.identity.%identitykey%.reply_on_top"/>
>+  <label hidden="true" wsm_persist="true" id="identity.autoQuote"
>+        pref="true" preftype="bool" prefattribute="value"
>+        prefstring="mail.identity.%identitykey%.auto_quote"/>
You should be able to put the wsm_persist and id on the checkboxes and
menulist, so that the wsm will save the prefs for you.

>-  else
>-  {
>-	document.getElementById("useGlobalPref").removeAttribute("disabled");
>+  else {
>+    document.getElementById("useGlobalPref").removeAttribute("disabled");
Please don't change brace style; changing whitespace is acceptable if you also
provide a diff -w

>-      document.getElementById("directoriesList").getAttribute('value');
>+      document.getElementById("directoriesList").getAttribute("value");
Pointless.

Comment 26

[Ian Neal]

Attachment: Patch v0.5d generated using pud8

Addressing issues identified by Neil.
Most whitespace changes are to correctly the line up of the code within braces
or to replace tabs with spaces.
New uuids created for changes in interfaces.
Inlined my setupQuoteList as it wasn't being called from anywhere else.

Comment 27

[Ian Neal]

Attachment: Patch v0.5d generated using wpud8

Same as Patch v0.5d except generated using extra w

Comment 28

[[e-mail]]

Comment on attachment 129095 [details] [diff] [review]
Patch v0.5d generated using wpud8

>+  var quoteList = document.getElementById("identity.replyOnTop");
>+  if (quoteList.value == "")
>+    quoteList.setAttribute("value", 1);
What's the point of this?

Comment 29

[[e-mail]]

Comment on attachment 129095 [details] [diff] [review]
Patch v0.5d generated using wpud8

> NS_IMETHODIMP
> nsMsgCompose::ConvertAndLoadComposeWindow(nsString& aPrefix,
>                                           nsString& aBuf,
>                                           nsString& aSignature,
>+                                          nsIMsgIdentity *identity,
>                                           PRBool aQuoted,
>                                           PRBool aHTMLEditor)
Well, there are two errors here. The minor error is not using the a prefix for
an argument. The major error is that an nsMsgCompose object already has an
m_identity variable, which is the one that was used to construct the
QuotingOutputStreamListener, that you're carefully sending back :-P

Comment 30

[Ian Neal]

Hmmm, re comment#28 there did used to be certain circumstances that produce a ""
value but I can't get it to happen any more, perhaps it was when I forgot to
reset my pref settings during testing.

RE comment#29 ConvertAndLoadComposeWindow is called from two different places
one with m_identity defined and the other with mIdentity defined hence passing
it the way I did and the way ProcessSignature also passes it (which is where I
looked at how to define the arguments to pass and where I got *identity from
unless things differ between NS_IMETHODIMP and nsresult functions).

Comment 31

[Ian Neal]

Attachment: Patch v0.5e generated using pud8

Using m_identity and not altering nsMsgCompose interface

Comment 32

[Ian Neal]

Attachment: Patch v0.5e generated using wpud8

As patch v0.5e excepted generated using added w

Comment 33

[[e-mail]]

There's just one issue that comes to mind... what about all the existing users
with non-default preferences, they'll have to reset them, for each identity...

Comment 34

[Ian Neal]

Hmmm, good point, any ideas what has been done when things have been moved
previously?

Comment 35

[jay garcia]

Re: Comment #33

Users *should* read the release notes and *should* understand the caveats of
alpha/beta software as well as willing to accept the changes therein. Those that
most want the changes will be happy campers, those that don't still *should* be
content to have non-global per-account basis control. There will *always* be
those that moan and groan no matter what.

Comment 36

[Ian Neal]

One option is to include a script that copies the preferences across to each
account and then deletes the old preferences, just not sure the best place for
that script to sit.
Another is to use the old preferences until the user goes in and sets new
preferences but, to me, that seems less tidy.

Comment 37

[jay garcia]

Re: Comment #36

Unless I'm missing your point, giving the user *more* control over the
per-account preferences and then taking them away with a script seems
counter-productive. Leave total control in the hands of the user, that's what
they beg for, IMHO.

Comment 38

[Ian Neal]

My prefered option would be for the user to have to set their own per account
preferences, but if that is not acceptable then for the global preference to be
copied across to each account and then the global preference deleted and all
subsequent changes are made on a per account basis.

Comment 39

[[e-mail]]

jay garcia, the problem is that this patch will effectively remove the global
preference from mailnews.reply_on_top replacing it with a default preference
mail.identity.default.reply_on_top thus anyone who changed their global
preference will think that their change has been lost, and might even want some
UI to change the new default preference (to which I would say about:config ;-)

Comment 40

[[e-mail]]

Comment on attachment 129133 [details] [diff] [review]
Patch v0.5e generated using wpud8

Well I would be happy enough if this was relnoted to say "Each identity now has
its own quotation preferences which you will have to set up individually even
if you had changed quotation preferences in the past."

P.S. how did you get moz to rebuild after changing the uuid? it royally horked
my build and I had to remove that from the patch :-/

Comment 41

[Ian Neal]

Adding relnote keyword

As far as rebuilding, I did a |make clean| in mailnews and then a |make -f
client.mk build| in the root of the tree.

Comment 42

[Ian Neal]

Comment on attachment 129133 [details] [diff] [review]
Patch v0.5e generated using wpud8

Requesting superreview

Comment 43

[Ian Neal]

Targetting

Comment 44

[David :Bienvenu]

this looks OK to me, except I think it's worth a try to upgrade people's prefs.
The way we used to do stuff like this is with a version pref for prefs.js. Now,
we seem to add a pref that's specific to the feature being upgraded, in this
case, something like "mailnews.quotingPrefs".

so, in mailnews.js, you'd add
pref("mailnews.quotingPrefs.version", 0);

then, in some startup code, where we load accounts, you'd get that pref. If it
was 0, then you'd apply the mailnews.reply_on_top pref to all accounts.
Actually, you'd only need to apply it if it wasn't the default, I guess. You'd
need to use catch + try to handle the case where the pref wasn't in prefs.js
(and thus was the default). Then, you'd update the version pref to 1 so we
wouldn't try to upgrade again.



we don't change uuid's when an interface has things added to it, generally. It's
not going to help anyone, and it's going to mess people up who pull and build
the tree. I'd remove that change.

Comment 45

[[e-mail]]

One approach would be to try and query the old pref, and if it exists then apply
it to all accounts and delete the old pref. Perhaps in verifyAccounts()?

Comment 46

[Peter Lairo]

Re. Comment #23 
> move the compose HTML preference from the main panel to the new
> "Addressing & Composition" panel) 

Good idea. The *defaults* for new accounts should be: 
- e-mail account:    HTML-yes
- newsgroup account: HTML-no

I can't remember where this was brought up, but... The <account-name> preference
panel should only contain settings for things that are needed and don't/can't
have a sensible default (priciple: don't bug the user with settings that don't
*have* to be set). Therefore, we should:

A. keep the "Attach this Signature" in the main (<account-name>) panel.
B. Move "Compose in HTML" to "the "Addressing & Composition" panel.
C. Put the "Forward" (inine/attachment) and "Reply" (above/below/...) in the
"Addressing & Composition" panel.

PS. I wonder when people will ask to have "Spell-Check" and "Font" prefs on a
per-account basis. :-\ (but that should definetely be a separate bug)

Comment 47

[Ian Neal]

Re: comment#46
Peter could you log separate bugs, if they don't already exist, on the issues
not covered by this bug such as different defaults for HTML composition
depending on account type and moving other preferences to be on a per account basis.

Comment 48

[Peter Lairo]

Ian, I just tested it (new e-mail and news accounts) and those defaults already
exist. No need for a new bug. 

Which "other preferences" were you referring to? The only thing not yet
discussed would be also moving the "Forward" (inine/attachment) setting. You
really want that in a separate bug? I can gladly do that. Alternatively, I could
just rename the summary here to include Forwarding (my preference).

Comment 49

[Ian Neal]

Yes I would want "Forwarding" in a separate bug so the pros and cons can be
discussed there.

Comment 50

[Peter Lairo]

OK, I just filed bug 215252. Come to think of it, I can't see any reason for
that bug at the moment. Hopefully, someone will come up with a reason why users
might want forwarding "inline" in some accounts and "as attachment" in other
accounts. :-\

Comment 51

[Ian Neal]

Attachment: accountUtils.js patch v0.1

Patch to accountUtils.js to migrate global quoting preferences to per account
ones.

Comment 52

[[e-mail]]

Comment on attachment 129287 [details] [diff] [review]
accountUtils.js patch v0.1

>+    deletePrefs = true;
No need for this, just leave everything in the try/catch; if the prefs got
removed last time then this code will automatically be skipped.

>+    if (!auto_quote || (reply_on_top != 0)) {
Probably unnecessary, if it's combined with the last patch.

>+      var numAccounts = accounts.Count();
>+      var numIdentities = 0;
>+      for (var i = 0; i < numAccounts; i++) {
>+        var account = accounts.QueryElementAt(i, Components.interfaces.nsIMsgAccount);
>+        var identities = account.identities;
Actually I think the account manager holds a list of all identities, you don't
need to check each account in turn.

>+              pref.setIntPref("mail.identity."+identity.key+".reply_on_top", reply_on_top);
Surely identity.reply_no_top = reply_on_top; will do?

Comment 53

[David :Bienvenu]

the problem with doing it this way (deleting the original pref after upgrade) is
that if you use an older, stable version against the same profile, you'll lose
your original settings. In this case, it's probably not a big deal, but just
FYI. The versioning scheme doesn't have this problem. But, as I said, no big
deal, and this approach is OK.

Comment 54

[Ian Neal]

Using identity.reply_no_top = reply_on_top gives me the following exception:

Exception... "Cannot modify properties of a WrappedNative" nsresult: "0x80570034
(NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)

Revised patch coming soon...

Comment 55

[Ian Neal]

Attachment: Revised accountUtils.js patch v0.2

Comment 56

[Ian Neal]

Comment on attachment 129312 [details] [diff] [review]
Revised accountUtils.js patch v0.2

not included the addition to mailnews.js but I will include that in the final
patch for superreview

Comment 57

[[e-mail]]

Comment on attachment 129312 [details] [diff] [review]
Revised accountUtils.js patch v0.2

>         var am = Components.classes[accountManagerContractID].getService(Components.interfaces.nsIMsgAccountManager);
> 
>         var accounts = am.accounts;
> 
>+        // migrate quoting preferences from global to per account
>+        migrateGlobalQuotingPrefs(am.allIdentities);
>+
>         // as long as we have some accounts, we're fine.
>         var accountCount = accounts.Count();

Put migrateGlobalQuotingPrefs before var accounts for r=.

Comment 58

[[e-mail]]

Comment on attachment 129312 [details] [diff] [review]
Revised accountUtils.js patch v0.2

Wait a sec, are you sure that identity.reply_on_top = reply_on_top doesn't
work?

Comment 59

[[e-mail]]

Comment on attachment 129312 [details] [diff] [review]
Revised accountUtils.js patch v0.2

Silly me, I meant identity.replyOnTop = reply_on_top :-[
Also I'm not sure why you set the quoting version in a try/catch.

Comment 60

[Ian Neal]

try/catch was left over from when I was attempting to delete preferences and I
forgot to remove it.
I still get the same exception if I use identity.replyOnTop = reply_on_top;

Comment 61

[Ian Neal]

Attachment: Patch v0.5f includes migration

Takes on board all comments and migrates global prefs to per account prefs

Comment 62

[David :Bienvenu]

Comment on attachment 129380 [details] [diff] [review]
Patch v0.5f includes migration

sr=bienvenu

One tiny nit, however:

this code is too clever by half :-)

+    if (!auto_quote || (reply_on_top != 0)) {
+      var numIdentities = allIdentities.Count();
+      var identity = null;
+      for (var j = 0; j < numIdentities; j++) {
+	 identity = allIdentities.QueryElementAt(j,
Components.interfaces.nsIMsgIdentity);
+	 if (identity.valid) {
+	   if (auto_quote)
+	     identity.replyOnTop = reply_on_top;
+	   else
+	     identity.autoQuote = false;
+	 }
+      }
+    }

even if auto-quote is turned off, you still want to carry forward the reply on
top pref to be consistent, because it's the value that's used by default if
auto-quoting is turned on.

Also,	  if (!auto_quote || (reply_on_top != 0)
this can just be !auto_quote || reply_on_top)

Comment 63

[Ian Neal]

Attachment: Patch v0.5g

Extra nits dealt with patch

Comment 64

[Ian Neal]

Attachment: Patch v0.5g against correct tree

Oops, diff'd against wrong tree this is against the right tree.

Comment 65

[Ian Neal]

Comment on attachment 129476 [details] [diff] [review]
Patch v0.5g against correct tree

Carrying r+ and sr+ forward and asking for driver approval to check into 1.5b

Comment 66

[[e-mail]]

Comment on attachment 129476 [details] [diff] [review]
Patch v0.5g against correct tree

>+          if (auto_quote)
>+            identity.replyOnTop = reply_on_top;
>+          else {
>+            identity.autoQuote = false;
>+            identity.replyOnTop = reply_on_top;
>+          }
Still too clever? how about:
identity.autoQuote = auto_quote;
identity.replyOnTop = reply_on_top;

Comment 67

[David :Bienvenu]

yes, Neil, that's exactly what I would have expected to see - the best course
here is to have the least amount of code, not the most efficient code, since
this is code that executes only once. And I'd think that your suggestion is
probably the fastest as well as the smallest.

Comment 68

[Asa Dotzler [:asa]]

Comment on attachment 129476 [details] [diff] [review]
Patch v0.5g against correct tree

a=asa (on behalf of drivers) for checkin to Mozilla 1.5beta.

Comment 69

[[e-mail]]

Fix checked in.

Comment 70

[Peter Lairo]

Wouldn't it be better to have the "composition" settings *above* the
"Addressing" settings, since they are much more commonly used (especially "HTML
Y/N")? € 0.02

Comment 71

[Ian Neal]

Would the panel have to be renamed Composition & Addressing as well? I'd say it
would, all fairly easy to change though. Log a new bug and cc me if you want.

Comment 72

[Peter Lairo]

OK, I just filed bug 216533 for this.

Comment 73

[Mike Cowperthwaite]

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