1639452 - rework ReplyIgnoreQuote to be a boolean parameter quote, passed to MailServices.compose.OpenComposeWindow

Status: RESOLVED FIXED

(MailNews Core :: Composition, task)

Description

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/cfa832291ae06d6b814f8a9ebe1991a08f25da92/mailnews/compose/public/nsIMsgComposeParams.idl#57

We should never be messing with the compose type. Quoting or not should be a boolean parameter instead.

Comment 1

[Magnus Melin [:mkmelin]]

Not doing so would lead to further hacks e.g. for bug 1615996.

Comment 2

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

• Removed ReplyIgnoreQuote from nsIMsgCompType
• Added ignoreQuote param to OpenComposeWindow

I will push to Try to see if it breaks anything.

Comment 3

[Ping Chen [:rnons] (away)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=804166e0dc4f8fc5af36b79d65b7a8f821c329f3

There is a failure, but seems to be known issue.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9153298 [details] [diff] [review]
1639452.patch

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

::: mailnews/compose/public/nsIMsgComposeService.idl
@@ +27,5 @@
>                           in MSG_ComposeFormat format,
>                           in nsIMsgIdentity identity,
>                           in AUTF8String from,
> +                         in nsIMsgWindow aMsgWindow,
> +                         in boolean ignoreQuote);

I think it would be preferable to have this parameter be optional, and name it better. Like

```
[optional] in boolean suppressReplyQuote
```

In general it's preferable to have optional parameters default to false.

When it's optional you only need to pass it in for the few cases we want to set that parameter.

Please also add a documentation comment in the .idl file

Comment 5

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

• Renamed ignoreQuote to suppressReplyQuote
• Added some documentation to nsIMsgComposeService.idl

I thought about using optional parameter at first, but seems in most cases (compose types), we want to suppress reply quote. So even it's optional, we still often need to pass true. Unless we want to reverse this param to includeReplyQuote.

Comment 6

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

Lint.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Ping Chen (:rnons) from comment #5)

I thought about using optional parameter at first, but seems in most cases (compose types), we want to suppress reply quote.

Well, in most cases it's just not relevant. It only does something for the reply types and is ignored for all others. https://searchfox.org/comm-central/rev/771b2328aa3096e6c907f477d7ebd862c8bf0841/mailnews/compose/src/nsMsgComposeService.cpp#410-416
That's why it would be nice to be optional - not having to specify it when it's irrelevant. From a quick look at the patch, 7 or so of the cases are such that it's irrelevant.

Comment 8

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

Thanks, I see what you mean now. Fixed.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9154096 [details] [diff] [review]
1639452.patch

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

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +323,5 @@
>    aFormat,
>    aIdentity,
>    aMsgWindow
>  ) {
> +  let suppressReplyQuote = true;

Not completely sure now, but shouldn't you pass this in around here: https://searchfox.org/comm-central/rev/b3dc7ad59deb6d0e37499ed167d93c467fb1ccb0/mail/base/content/mailCommands.js#244 (I guess this case was forgotten before) and actually use it?

Or at least it was always false before, and then you'd just use this parameter for rss.

Comment 10

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

I should read the code more carefully. Changed to pass suppressReplyQuote to openComposeWindowForRSSArticle now.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9154534 [details] [diff] [review]
1639452.patch

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

::: mail/base/content/mailCommands.js
@@ +241,5 @@
>              type,
>              format,
>              identity,
> +            msgWindow,
> +            suppressReplyQuote

Sorry, hadn't thought this through earlier. We don't allow replying to feeds, so this parameter should always be irrelevant for feeds and we don't want to pass it in there.

(Or, I notice replies are erroneously enabled in the context menu, but even then the parameter will be irrelevant since actual quoting the same ways as mails don't happen.)

Comment 12

[Ping Chen [:rnons] (away)]

Attachment: 1639452.patch

Hope I get it right this time. Yes, the "Reply" items in the context menu of feeds confused me.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9154833 [details] [diff] [review]
1639452.patch

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

Looks good, thx! r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ece38adeaaee
Change ReplyIgnoreQuote to be a boolean parameter. r=mkmelin