Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Quote just the selected portion of a message during Reply

VERIFIED FIXED in mozilla1.9

Status

MailNews Core
Composition
P3
enhancement
VERIFIED FIXED
18 years ago
5 years ago

People

(Reporter: John Moreno, Assigned: Jeff Beckley)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
mozilla1.9
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

18 years ago
This is one of the GNKSA SHOULDs that earlier versions of Netscape "failed".
Quoting just part of a message makes it easier to focus a reply on just the
relevant portion.
(Reporter)

Updated

18 years ago
Blocks: 12699

Updated

18 years ago
Summary: Quote just the selected portion of a message → Quote just the selected portion of a message

Comment 1

18 years ago
cc: phil.  Phil, didn't you file bugs for a bunch of the gnksa items already?

Updated

18 years ago
Status: NEW → ASSIGNED
Target Milestone: M18
No Idea if we will do it, will see...could be nice...

Comment 3

18 years ago
Implementation note: If the user selects some of the message, the text of the 
menu item (and the toolbar button tooltips) should change from `Reply to Message' 
to `Reply to Selected Text'.

Note that if the bug for snipping a signature on reply is fixed, this bug should 
be fixed at about the same time, so that a user can still include the signature 
in their reply (for the purposes of commenting on the sig or whatever) -- by 
pressing Ctrl+A (select all), then Ctrl+R (reply to selected text).

Updated

17 years ago
QA Contact: lchiang → pmock

Comment 4

17 years ago
<quote src="http://www.xs4all.nl/%7Ejs/gnksa/gnksa.txt">
As a direct counterpart to this requirement, the software SHOULD offer
the user some means of selecting exactly which part of a Usenet posting
she wishes to followup to, and quote only that part initially.  (A
special case of this is when a user wishes to react to what's in a
signature.)
</quote>

BTW: A workaround would be "select - copy - reply - select all - delete - paste
as quotation". It does the same, but is a long sequence.

> Note that if the bug for snipping a signature on reply is fixed, this bug
> should be fixed at about the same time, so that a user can still include the
> signature in their reply (for the purposes of commenting on the sig or
> whatever) -- by pressing Ctrl+A (select all), then Ctrl+R (reply to selected
> text).

Agreed, but live was different :-(. I agree, that we should fix this bug as soon
as reasonable.

Unfortunately, the implementation involves lots of JS/DOM stuff, I guess, so I
won't be able to do much for it :-(.

Comment 5

17 years ago
I looked into this once, hoping there might be a quick fix.  The problem I ran
up against was that quoting in the message window is asynchronous and comes from
the mail database, not from the DOM in the message window, whereas the selection
is against the message window DOM.

Now that it's possible to get the selection in JS from a Window or Document
object, it might be easier to implement this purely in JS: if you can get the
window corresponding to the message pane content, then you can get the selection
from that, do a ToString on it, bring up a new compose window (no reply), then
do an InsertAsQuotation, and not even need to go into C++ code.  Alas, I had to
go work on dogfood/beta bugs and ran out of time to play around with this. 
Perhaps these suggestions will help someone else ...

Comment 6

17 years ago
Moving to future milestone. This would be really nice to have, but I don't think 
we'll have time to get this in.
Target Milestone: M18 → Future

Comment 7

17 years ago
Looks like the helpwanted keyword got lost -- this should clearly be a
helpwanted bug.
Keywords: helpwanted

Comment 8

17 years ago
> bring up a new compose window (no reply)

huh? why not a reply? If reply, that's exactly the problem with your proposal:
You won't easily get the headers etc.. So, I think, this bug would involve C++
hacking (nsMsgCompose, I think).

Comment 9

17 years ago
Not a reply because then the C++ code wouldn't auto-insert the whole quoted
material, and we'd be free to get the contents of the selection and insert it
ourselves from JS (once the window came up).

Comment 10

17 years ago
But you loose all of the reply-info: recipients, threading etc..

I don't think, this bug is hard to fix. It just crosses a lot of code (UI,
nsMsgCompose, possibly nsIMsgCompose).

Comment 11

16 years ago
*** Bug 89610 has been marked as a duplicate of this bug. ***

Comment 12

16 years ago
*** Bug 107553 has been marked as a duplicate of this bug. ***

Comment 13

16 years ago
Wow, its been over a year since this has been looked at ... in that time, many
things have changed, I suppose.  What's the current take on how difficult this
would be?

Updated

16 years ago
QA Contact: pmock → sheelar

Comment 14

16 years ago
*** Bug 69129 has been marked as a duplicate of this bug. ***

Comment 15

16 years ago
*** Bug 129650 has been marked as a duplicate of this bug. ***

Comment 16

16 years ago
*** Bug 131365 has been marked as a duplicate of this bug. ***

Comment 17

15 years ago
I am very surprised this feature request is so old...

It is an extremely useful feature that most mailers have...

Also, a 'Paste as quotation' would also be very nice, once this feature is
implemented, so that *any* copied/pasted text could be pasted as a quotation...
or, at the very least, the ability to 'quote only selected text' with
discontiguous blocks of text in a message.

In addition (and similarly), the ability to re-wrap blocks of text, cleaning up
the quoting characters, would be awesome!  I have sorely missed this feature
that I have only seen in Outlook Express on the Mac.

Would it be possible to 'borrow' code from some other project?

I know of one Open Source mailer that does this very well - Sylpheed - but it is
done in GTK+...

*Please* bump this up in priority, guys - I would really love to start using
Mozilla and Mozilla Mail as our company's exclusive Browser/Mailer of choice.

Thanks

Charles Marcus
I.T. Director
Media Brokers International

Comment 18

15 years ago
> Paste as quotation

Exists.

> the ability to re-wrap blocks of text, cleaning up
> the quoting characters

Exists, independently for both HTML composer and plaintext composer.

Comment 19

15 years ago
Well, I posted this feature request on the n.p.m.wishlist newsgroup, and someone
pointed out that this feature request was an old one - and boy was I surprised
to find out it was over two *years* old - thats a long time to wait for this
functionality.  After reading all of the comments on it, I decided to submit a
new one with a couple of additional-but-related feature requests of mine, as a
new issue, and mark the old one (this one) as a duplicate of the new one.  I
hope I am not out of line doing this - this is my first foray into the Bugzilla
world.  If this was not the proper way to do this, my apologies...

Comment 20

15 years ago
>> Paste as quotation

>Exists.

I know - I just realized it... man, must be getting old...

>> the ability to re-wrap blocks of text, cleaning up
>> the quoting characters

> Exists, independently for both HTML composer and plaintext composer.

Doesn't work - at least, not for me.  I putposefully messed up some paragraphs,
and it didn't matter if I selected the paragraph in question ot not, Rewrap
didn't do anything.

Maybe I am not using it properly?

Comment 21

15 years ago
*** Bug 148669 has been marked as a duplicate of this bug. ***

Updated

15 years ago
QA Contact: sheelar → esther

Comment 22

15 years ago
There should be a keyboard combination for 'paste as quote'. 

I use selective quotes all the time, and would like to be able to use the
keyboard, not the mouse.
-->varada
Assignee: ducarroz → varada
Status: ASSIGNED → NEW

Comment 24

15 years ago
Betsy: that would be a separate bug entirely.

Comment 25

15 years ago
I got carried away with the power of writing to the bug list - i'm just an
ordinary consumer not a programmer :-)

I've written to feedback about it. Can i report it as a bug? It really annoys me.

Comment 26

15 years ago
Betsy: If you have a middlemouse button and set the "middlemouse.paste" pref (on
by default on linux, off on the other platforms), then ctrl-middleclick does a
paste as quotation.  But you'd have to file another bug to request a key binding
for it.

Comment 27

15 years ago
It turns out that this is possible but there is no menu entry.

This is a quote that was posted in Bug 100567:
http://bugzilla.mozilla.org/show_bug.cgi?id=100567

"Additional Comment #4 From Dave North  2002-06-25 18:24 -------

... just discovered (on OS X) that apple-shift-v does the trick. My "bug" would
be: no accelerator listed in the menu, even though there is one.

Windows ... ctrl-shift-v.

... Linux ctrl-shift-v .

So it looks like all that needs to be done is including a menu entry to let the
userverse know this."

Comment 28

15 years ago
Ctrl-Shift-V = Paste As Quotation. So, there is a menu entry, but it's not this
bug (see comment 4). (The keyboard shortcut is not in the menu, that's true, but
again, it's not this bug.)
Changing summary to narrow bug.
Summary: Quote just the selected portion of a message → Quote just the selected portion of a message during Reply
taking all of varada's bugs.
Assignee: varada → sspitzer

Comment 30

15 years ago
In response to all the comments helpfully pointing out the "Paste as Quotation"
feature: yes, I know about that, but quoting just the selected portion is more
efficient than Ctrl-C to Copy, Reply, Select All, Ctrl-V to Paste. I regret that
I don't speak C++, but figure all that is needed is one snippet of code, which,
in a scripting language would look something like this:

If (NOT ?SelectedText)
EditSelectAll()
EndIf
vSource = ?SelectedText
[Existing code to switch to compose window, insert recipient, go to text entry box]
PasteasQuotation (vSource)

Seems to me like one little conditional statement is all that would be needed: a
fairly trivial addition to save a few steps in responding to e-mails. Also, it
would work the same way in Usenet replies, where often one wishes only to
respond to a portion of a message. "Paste as Quotation" is a nice workaround,
but it's still just a workaround.

Comment 31

15 years ago
> vSource = ?SelectedText
> [Existing code to switch to compose window, insert recipient, go to text entry
box]
> PasteasQuotation (vSource)

Well, yeah, but one of the problems, as so often, is to pass around that
vSource. Which means that it's way more than "one snipplet of code".

Comment 32

15 years ago
My apologies. I just wanted to point out, as others did a couple of years ago,
that other e-mail clients and/or newsreaders have this functionality, and that
Paste As Quotation, in my personal opinion and preferences, is not the solution
to this issue. Cheers.

Comment 33

15 years ago
I concur with Michael's statements. And while it may be difficult passing around
vSource, let's not shoot down a commenter just because he isn't a C++ guru. In
the grand scheme of Mozilla development, this functionality is a
drop-in-the-bucket, with respect to difficulty. And while it may not be as easy
as presented by Michael, it isn't as difficult as the inaction on this request
might indicate.

On the other hand, it should be understood that there may be more important bugs
that are requiring developer's attention at the moment. To make this bug "more
important", in a way - vote for it.

Cheers.

Comment 34

15 years ago
I also concour with  Michael A. Koenecke, that this is a necssity for a
competent mail client. Even OSS clients like Sylpheed have had this for at least
a year. If I was a competent programmer, I wouldn't hesitate to do it myself.
Unfortunately not all of us are - I'm simply a lowly beta tester, who
contributes the best way I can. Find bugs and request needed features.

Please Seth Spizer, I know you're overworked but we need this. I think it's
telling that somebody of John Moreno acutally opened this.

Cheers,
krutch
(Reporter)

Comment 35

15 years ago
Seth, given that you yourself reported a duplicate of this bug, any idea as 
to whether or not you'll actually be able to work on it?

Comment 36

15 years ago
*** Bug 193370 has been marked as a duplicate of this bug. ***

Comment 37

14 years ago
I agree that this is an important enhancement to make.  It is especially useful
for replying to digest mail where you just want to quote one of the messages but
there are many other uses.  Please implement it.

Comment 38

14 years ago
I recently implemented a patch for the QuickReply extension to Thunderbird that
enables you to quote just the selection:
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&threadm=bgbafm%24lp7%241%40ctb-nnrp2.saix.net&rnum=1&prev=/groups%3Fq%3Dgroup%253Anetscape.public.mozilla.mail-news%2Bquickreply
Anyone interested in this bug might like to try this out.

One of the real frustrations of trying to implement this patch was that I looked
at the standard Thunderbird coding for replies, which goes through a complicated
mechanism of loading the original message from the message database... it seems
there is no easy way (without stream listeners, etc) to just get the message body.
Having looked at this bug, I think the real reason that it hasn't been fixed is
that the whole reply mechanism is too complicated. If somebody looked at the
whole thing and simplified it, it would be fairly easy to do the
reply-to-selection. It was much easier to implement for quick-reply by just
getting the message pane frame, which you can ask for the selection. Look at the
patch if you're interested in details.

Comment 39

14 years ago
*** Bug 223094 has been marked as a duplicate of this bug. ***

Comment 40

14 years ago
*** Bug 223098 has been marked as a duplicate of this bug. ***

Comment 41

14 years ago
This bug is something that would be VERY helpful to the people I support.  It is
not uncommon to have huge messages that include some personal and some business
correspondence.  The user then wishes to forward only part of the message to
someone else without going through a copy/paste routine.  While this is a
work-around, the inability to print/forward/reply selected automatically is a
major inconvenience.  These users have had mailers in the past the offer this
function, so it is an expected feature when they move to MozMail.  They let me
know that they miss it, and if they care enough to call me and ask how to do it,
then I know that it consciously bothers them.  Please look into implementing
this.  From my standpoint, it should be a matter of Mail understanding that some
text in the body is highlighted, and when a forward/reply/print button is
pressed, only move the selected text.  Options regarding inline/attachment and
respond above/below quote should be honoured.

Comment 42

14 years ago
This would be a great feature for people who only like replying to one specific
portion of a message. Even a preference that would allow this would be awesome.

Thanks for considering it!

Comment 43

14 years ago
Other than the Quickreply extension, is there currently a workaround to enable
this behavior? Quickreply is a nice piece of software, but it isn't quite
"right" for me (no offense).

Any hacks or suggestions?

Comment 44

14 years ago
Select, Ctrl-C (Copy), Ctrl-R (Reply), Ctrl-Shift-Home (select everything),
Shift-Cursor-Down (unselect atribution line), Delete, Ctrl-Shift-V (Paste as
Quotation)

Comment 45

13 years ago
I just wanted to add another "please" to this bug. I just switched over from
PMMail2000, and quoting only the selected parts of a message really speeds up
the reply process. 

In fact, it's the one and only fact, I miss the most about Mozilla Mail.
Unfortunately I'm not a programmer, so I can't help with coding that stuff, but
hey, I'll be able to provide a lot of encouragement, good karma and stuff.. :-)

So please pick this bug up again. A lot of users will thank you for it - even if
they don't know it by now.

Best, Bjoern

Comment 46

13 years ago
*** Bug 249267 has been marked as a duplicate of this bug. ***

Comment 47

13 years ago
*** Bug 257404 has been marked as a duplicate of this bug. ***
*** Bug 268106 has been marked as a duplicate of this bug. ***

Comment 49

13 years ago
and make it appear in an alternate color with an introduction stating:

"Message Text in <insert color here> written by <insert name here>".
Product: MailNews → Core

Comment 50

13 years ago
*** Bug 272898 has been marked as a duplicate of this bug. ***

Comment 51

13 years ago
For anyone interested:

Thunderbird 1.0 with the 'Reset Quote Headers' extension now provides this
functionality, as does the 'QuickReply' extension, each with one caveat:

'Reset Quote Headers' works, but only if you are replying from the main window, and

'QuickReply' works only if replying from the QuickReply textbox.

Charles

Comment 52

13 years ago
For anyone interested:

Thunderbird 1.0 with the 'Reset Quote Headers' extension now provides this
functionality, as does the 'QuickReply' extension, each with one caveat:

'Reset Quote Headers' works, but only if you are replying from the main window, and

'QuickReply' works only if replying from the QuickReply textbox.

Charles

Updated

13 years ago
Whiteboard: Workarounds in comment #52

Comment 53

13 years ago
(In reply to comment #52)
> Thunderbird 1.0 with the 'Reset Quote Headers' extension now provides this
> functionality, as does the 'QuickReply' extension, each with one caveat:

OK, I can find QuickReply at http://quickreply.mozdev.org/ (which does not
appear on the "active projects" list) but can't find "Reset Quote Headers"
anywhere. Link?

Comment 54

13 years ago
My apologies - should have procided links...

QuickReply (repackaged for TBird 1.0):
www.extensionsmirror.nl/extthunderbird/QuickReply_0.5.5_bump.xpi

Reset Quote Headers:
www.extensionsmirror.nl/extthunderbird/Reset_Quote_Header_0.2.8.xpi

www.extensionmirror.nl is a great place for extensions, if you didn't already
know...

Charles

Comment 55

13 years ago
*** Bug 281400 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Blocks: 163993

Updated

12 years ago
No longer blocks: 163993

Comment 56

12 years ago
*** Bug 299823 has been marked as a duplicate of this bug. ***

Comment 57

12 years ago
*** Bug 305549 has been marked as a duplicate of this bug. ***

Comment 58

12 years ago
(In reply to comment #54)
> 
> QuickReply (repackaged for TBird 1.0):
> www.extensionsmirror.nl/extthunderbird/QuickReply_0.5.5_bump.xpi

IMHO "quickreply" is not exactly the same as the original (now over 5 years
old!) proposal, though there is overlapping functionality.  Would be much better
to see this as part of the mailclient core.

Comment 59

12 years ago
You are correct - however, there is a new extension that I like even better, and
*is* much closer to what the original requst was asking for:

http://quickquote.mozdev.org/installation.html

Comment 60

12 years ago
*** Bug 312198 has been marked as a duplicate of this bug. ***

Comment 61

12 years ago
Is this EVER going to make into the core code???????

Comment 62

12 years ago
Yes. The new century has only just begun. :-P

Comment 63

12 years ago
That would be a nice feature, i think its kinda shame that this still doesnt work. I really love how this is implemented in sylpheed!

Comment 64

12 years ago
QuickQuote is now even better - you can integrate its functionality with the 'Reply' and 'Reply All' toolbar buttons.

http://quickquote.mozdev.org/installation.html

So, install the extension, go to the options and check the boxes to integrate with the toolbar buttons, highlight some text in a message, and click a button.

It still isn't perfect, but it's getting there.

Hopefully this will get integrated into the core code soon. Then I'll only be complaining about the lack of a proper Siganture Manager (yes, I am aware of the two extensions (Signature and Signature_Switch) for this, but they don't work very well.

Comment 65

12 years ago
(In reply to comment #64)
> http://quickquote.mozdev.org/installation.html

Install fails in Mozilla; seems only to work with Firefox/Thunderbird?

Comment 66

12 years ago
Hi folks. This comment may seem a little out of context because it's not a complaint or even an observation about the progress of this project. I think the extension is great and it works well for me.

Fact is, I've been searching for some workaround to function as a REPLY-USING-TEMPLATE. As you know, templates in the current TBird are limited and awkward. This is where the search has led, to QuickQuote, because it has all the basic functions to reply to a message in a concise way. The only thing it really needs to provide users with a functional templates system is the ability to specify a file or template target into which the quoted text can be inserted (and to push a bit further- quote entire content). 

It's not evident when TB will get around to implementing a full templates system, but it seems to me and many of the forum posts made clear that templates are a primary function which should have been addressed long ago. 

So consider this a fervent appeal to broaden this extension a little, and gain a lot more utility. Many thanks for all your hard work. QuickQuote is great.

Comment 67

12 years ago
Well,

Im not sure if this is the right place to talk about the Quickquote extension. 

That's why I posted your comment at the author's homepage in the bugs section:

http://quickquote.mozdev.org/bugs.html

Best, Bjoern

Comment 68

12 years ago
*** Bug 326992 has been marked as a duplicate of this bug. ***

Comment 69

12 years ago
"Reset Quote Header" stopped working with 1.5, which is really annoying, for several reasons.
BTW, does anyone know if there's a bug for the inclusion of somewhat more verbose Reply headers? The normal ones in TBird make replied mails pretty useless in a professional setting (at least at my workplace).

Comment 70

12 years ago
You can change the Quote HEader with QuickQuote as well... it really does do a very good job of fulfilling this feature request... check it out:

http://quickquote.mozdev.org/

Comment 71

11 years ago
I am not a programmer, but is this feature so hard to implement?

Comment 72

11 years ago
With regards to : http://quickquote.mozdev.org/

It does an *ok* job of providing the behaviour - it doens't put the text cursor at the bottom of the mail (encouraging top posting, bad bad)

This should be built into the mail client, as the quickquote thing seems like a bit of a hack, it doesnt' replace the reply feature, it creates it's own, and you can see it selecting the main window, highlighting hte text, and presumably grabbing it and throwing it into the compose windows. (no offence to the author of this extention, you're doing a great job with what you have to work with I'm sure)


Comment 73

11 years ago
*** Bug 353829 has been marked as a duplicate of this bug. ***
I'm not working on this, but would really like to see it!
Assignee: sspitzer → nobody
QA Contact: esther → composition

Updated

11 years ago
Blocks: 213562

Comment 75

11 years ago
*** Bug 361435 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Blocks: 359227

Comment 76

11 years ago
I notice it's been SEVEN YEARS since people have requested a built-in solution to this. I hope somebody takes it up sometime! :)

doug
Duplicate of this bug: 381818

Comment 78

10 years ago
This is essential.

Comment 79

10 years ago
This is essential.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Assignee: nobody → beckley
Status: ASSIGNED → NEW
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 80

10 years ago
Created attachment 298363 [details] [diff] [review]
Implementation of reply quote selected

Here's an implementation of this feature.  It adds two new prefs.  "mailnews.reply_quoting_selection" controls whether or not the feature happens or not.  "mailnews.reply_quoting_selection.only_if_chars" is a list of characters, one of which must be present in the selection in order to quote just the selection on the reply.  The default value for the pref is newline, period, exclamation point, and quotation mark.  This is done so that other types of selection (e.g. find a word in a message) don't cause reply quoting selection to kick in.
Attachment #298363 - Flags: superreview?(mscott)
Attachment #298363 - Flags: review?(bienvenu)

Comment 81

10 years ago
Wow... excellent!! I'm really glad to finally see some action on this one... hopefully the patch is acceptable to the ones who decide whether or not to include it...

Comment 82

10 years ago
Thanks for implementing this important feature. Just looked at the patch superficially, but it looks very good (small and clean).

Comment 83

10 years ago
yeah, thx Jeff, it looks pretty good. I saw what looks like a tab:

+  {
+	nsString selPlain;

and the Lossy convert always raises a red flag, but it's probably ok for this. I need to try running with it, though.
(Assignee)

Comment 84

10 years ago
Oh, didn't notice the tab.  I try to keep those out, but it looks like one snuck in.

Yeah, the "lossy" bugged me at first, but I just thought users are not going to want to have anything but sentence-ending punctuation in that list.  I suppose I could have used UTF-8, but it seemed unnecessary.

Comment 85

10 years ago
Hmm... http://en.wikipedia.org/wiki/East_Asian_Punctuation
(Assignee)

Comment 86

10 years ago
Magnus, thanks for pointing that out.  So I'll change it to UTF-8.

Anyone know of a strcspn()-equivalent for UTF-8 strings?  That would be the cleanest, although it's not that much code to do it myself.

Comment 87

10 years ago
While your tinkering, remember changing the IDL's UUID. ;-)
(Assignee)

Comment 88

10 years ago
Created attachment 299227 [details] [diff] [review]
New patch from comments

Here's a new whole patch that address David, Magnus, and Karsten's concerns:

- Removed tabs
- Changed character list to use UTF-8 so East Asian punctuation can be handled
- Changed the UID on nsIMsgComposeParams.idl

Thanks guys!
Attachment #298363 - Attachment is obsolete: true
Attachment #299227 - Flags: superreview?(mscott)
Attachment #299227 - Flags: review?(bienvenu)
Attachment #298363 - Flags: superreview?(mscott)
Attachment #298363 - Flags: review?(bienvenu)

Comment 89

10 years ago
Comment on attachment 299227 [details] [diff] [review]
New patch from comments

if you can, +  attribute string                origQuoteHTML;

can you try using an ACString instead? That's currently preferred...

can you remove 

 pref("mail.imap.check_deleted_before_expunge", false);
+pref("mail.imap.expunge_option",            0);
+pref("mail.imap.expunge_threshold_number",  20);

from your patch?

With those changes, r=bienvenu. I'm glad this approach worked out!
Attachment #299227 - Flags: review?(bienvenu) → review+

Comment 90

10 years ago
 I'm glad to see this bug addressed, but I have a concern that the behaviour is made unnecessarily complex by "mailnews.reply_quoting_selection.only_if_chars".  

 Having the whole message quoted if there is no selection, and only the selection if one exists, is simple and intuitive behaviour. Having the selection quoted only under some circumstances, which the user isn't told about at the beginning, will be confusing at best (and downright annoying, typically). The principle of least surprise argues for the simple behaviour.

 Guessing which selections the user wants or doesn't want quoted in a reply is a losing game. Who can say that the user doesn't want a selection not containing "\n.!?" for quoting in a reply? For example, the current default will not quote selections which are sub-clauses with comma or semicolon punctuation, nor text in double quotes where the final punctuation mark is placed outside the closing double quote. Who can say that the user doesn't want a selection from word search to be prevented being quoted in a reply.

 As simple "quote only the selection if it exists" behaviour is least confusing, and doesn't prevent the user from obtaining selected text quoting they might legitimately expect to obtain, I ask that "mailnews.reply_quoting_selection.only_if_chars" and associated behaviour be dropped from the fix to this bug.

Comment 91

10 years ago
(In reply to comment #90)
>  Having the whole message quoted if there is no selection, and only the
> selection if one exists, is simple and intuitive behaviour.

I agree with comment #90. The only exception would be if the selection contains nothing but white space (potentially at different quote levels).

I believe this is how Eudora (<=7) does it; if only a single word is selected, that would be the reply quote.

Comment 92

10 years ago
> I'm glad to see this bug addressed, but I have a concern that the behaviour is
> made unnecessarily complex by "mailnews.reply_quoting_selection.only_if_chars".

Completely agreed. I suggest to remove the pref.

The only exception I *could* see would be whitespace only and a single word being selected, because this can happen with just a double-click and may not be intended. I would solve the Japanese problem by saying: If selection contains only whitespace or selection contains only [a-ZA-Z] (i.e. only latin characters, no umlauts, special chars or CJK chars), then don't use the selection. Otherwise use it.

Comment 93

10 years ago
> If selection contains only whitespace or selection contains only [a-ZA-Z]

Note: These are 2 different cases. If selection contains latin and whitespace, use it.
(Assignee)

Comment 94

10 years ago
(In reply to comment #90)
> Guessing which selections the user wants or doesn't want quoted in a reply is
> a losing game.

Yes, it is a heuristic, but it's one we found that worked very well in Eudora.  It fails much less than it succeeds.  I'd like to see the pref remain.  I suppose we could default it to be empty, which would indicate all selections are quoted, and then users could change it to what they want.


(In reply to comment #92)
> The only exception I *could* see would be whitespace only and a single word
> being selected

This could be a compromise.  The most common misfire on reply quoting selection is when the user searches for a word and then replies afterward.  The algorithm could be to remove leading and trailing whitespace from the selection, and then look for only non-East Asian characters in what remains.  This could be tied to a pref as well, which would be on by default.


The user then could decide to use and combination of these prefs.  And there is the global pref to turn off the feature entirely.

Comment 95

10 years ago
FWIW, the case that I'm thinking of that the current code (much less the default pref) does not cover is:
I do want to be able to quote just "hello world", but I don't want it to trigger for 1) whitespace only, 2) on a single word selection (double click) or 3) a part of a word (search, as you mentioned). The algorithm "trim leading/trailing whitespace, check that result contains something apart from a-z" (space => success) would achieve that. If you want to pref it, maybe allow to put a regexp in the pref? The above would be (if my regexp foo does not fail me) selection.match(/\s*[^a-zA-Z]+\s*/).

Comment 96

10 years ago
> If you want to pref it, maybe allow to put a regexp in the pref?

Oh, you're in C++, and regexps are hard from there (bug 48179, bug 348642).
(Assignee)

Comment 97

10 years ago
Created attachment 300204 [details] [diff] [review]
Now with multi-word check

Changes based on latest comments:

- Use of ACString instead of string in IDL
- Extraneous prefs removed
- Addition of "mailnews.reply_quoting_selection.multi_word" pref
- Default value of "mailnews.reply_quoting_selection.only_if_chars" now empty

"mailnews.reply_quoting_selection.multi_word" is a boolean pref that controls whether there have to be multiple words in the selection in order to quote just it on the reply (defaults to on).

I'd appreciate comments from i18n folks on whether my technique for determining multiple words in the selection is okay (see nsMsgComposeService::GetOrigWindowSelection), which is basically remove leading and trailing spaces, and then look to see if the selection contains only non-space single and 2-byte UTF-8 chars.
Attachment #299227 - Attachment is obsolete: true
Attachment #300204 - Flags: superreview?(mscott)
Attachment #300204 - Flags: review?(mkmelin+mozilla)
Attachment #299227 - Flags: superreview?(mscott)

Comment 98

10 years ago
Comment on attachment 300204 [details] [diff] [review]
Now with multi-word check

I only do /mail reviews.   Looks good to me though, unfortunately the patch has bit rot already:(

Since it needs an update anyway, remove the double empty line, put a . after the nsresult GetOrigWindowSelection documentation sentence. And I hear the ns(C)AutoString versions is preferred for local strings - someone correct me if I'm wrong.

Thanks for working on this though, it really is *the* feature I like in eudora!
Attachment #300204 - Flags: review?(mkmelin+mozilla)

Updated

10 years ago
Keywords: helpwanted
Duplicate of this bug: 419063
(Assignee)

Comment 100

10 years ago
Created attachment 307762 [details] [diff] [review]
Un bit-rotted and fixed reply all

This new patch is now using recent file revisions for diffs, and it addresses Magnus' comments.

It also fixes a bug where Reply All wasn't getting all of the recipients in the reply.
Attachment #300204 - Attachment is obsolete: true
Attachment #307762 - Flags: superreview?(mscott)
Attachment #300204 - Flags: superreview?(mscott)
(Assignee)

Updated

10 years ago
Attachment #307762 - Flags: superreview?(mscott)
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Don't you need superreview for this? bienvenu can do r+sr, but please ask him to mark it as such if you want him to... else get somebody like mscott to sr= it.
Re-add checkin-needed when you have all required reviews.
Keywords: checkin-needed
(Assignee)

Updated

10 years ago
Attachment #307762 - Flags: superreview?(dmose)
Duplicate of this bug: 423904
Comment on attachment 307762 [details] [diff] [review]
Un bit-rotted and fixed reply all

>+  attribute ACString              origQuoteHTML;

This name seems non-obvious on first reading.  htmlToQuote would be a bit clearer; there might be something clearer still.

Please add doxygen commentary here describing exactly what this new attribute means and how it is used.  directory/xpcom/public and calendar/public include examples of this sort of thing.

>+    mIgnoreData = PR_FALSE;

Instead of this mIgnoreData stuff, have a look at the call to mQuote->QuoteMessage in nsMsgCompose::QuoteMessage.  Currently, it's always called with a headersOnly value of PR_FALSE.  If you can tweak that code so that it gets called with headersOnly set to PR_TRUE in the case where we've got a selection, it should be possible to get rid of mIgnoreData.

Please also file a bug about disentraining the header setup in replies from the quoting, as having to call mQuote->QuoteMessage for the header-setting side effects is bogus.
Attachment #307762 - Flags: superreview?(dmose) → superreview-
(Assignee)

Comment 105

9 years ago
Created attachment 313632 [details] [diff] [review]
New patch addressing dmose's comments

Here's a new patch incorporating the changes that dmose suggested:
- Renamed origQuoteHTML to htmlToQuote
- Reworked the code to get rid of mIgnoreData
- Added doxygen-style comment to the new htmlToQuote attribute
Attachment #307762 - Attachment is obsolete: true
Attachment #313632 - Flags: superreview?(dmose)
Comment on attachment 313632 [details] [diff] [review]
New patch addressing dmose's comments

Looks good.  sr=dmose, conditional on getting input from some i18n folks on the UTF8 stuff, as well as tweaking a few nits:

>+
>+  /**
>+   * HTML-formatted content to quote in the body of the message.
>+   * Set this to get different content than what would normally
>+   * appear in the body, e.g. the original message body in a reply.
>+   */
>+  attribute ACString              htmlToQuote;
> }

htmlToQuote is encoded in UTF8, so this should be AUTF8String.  Otherwise if someone tries to access it from JS, bad things will happen.

> NS_IMETHODIMP QuotingOutputStreamListener::OnStopRequest(nsIRequest *request, nsISupports * /* ctxt */, nsresult status)
> {
>   nsresult rv = NS_OK;
>   nsAutoString aCharset;
> 
>+  if (!mHtmlToQuote.IsEmpty())
>+  {
>+    // If we had a selection in the original message to quote, we can add
>+    // it now that we are done ignoring the original body of the message
>+    nsCOMPtr<nsIInputStream> stream;
>+    rv = NS_NewCStringInputStream(getter_AddRefs(stream), mHtmlToQuote);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    mHeadersOnly = PR_FALSE;
>+    rv = OnDataAvailable(nsnull, nsnull, stream, 0, mHtmlToQuote.Length());

OnDataAvailable doesn't currently make use of the first two params, but at some point it might want to.  Better to pass them through rather than handing in nsnull, I'd think.

>+nsresult
>+nsMsgComposeService::GetOrigWindowSelection(MSG_ComposeType type, nsIMsgWindow *aMsgWindow, nsACString& aSelHTML)
>+{
>+  nsresult rv;
>+
>+  // Good hygiene
>+  aSelHTML.Truncate();
>+
>+  // Get the pref to see if we even should do reply quoting selection
>+  nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+  NS_ENSURE_TRUE(prefs, NS_ERROR_FAILURE);

Here and elsewhere, please use the two argument form of do_GetService and propagate rv up the stack.  In some situations, it will make debugging problems after this returns easier.


>+  nsCOMPtr<nsIDocShell> docShell(do_QueryInterface(childAsItem));
>+  NS_ENSURE_TRUE(docShell, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIContentViewer> contentViewer;
>+  rv = docShell->GetContentViewer(getter_AddRefs(contentViewer));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMDocument> domDocument;
>+  rv = contentViewer->GetDOMDocument(getter_AddRefs(domDocument));
>+  NS_ENSURE_SUCCESS(rv, rv);

Seems like the code would be slightly clearer on first reading if the above hunk were moved to just before where domDocument is actually used.

>+      for (; i != end; ++i)
>+      {
>+        char c = *i;

If you can, prefer |const char c| for optimizer hinting goodness...
Attachment #313632 - Flags: superreview?(dmose) → superreview+
Adding m_wada and jshin in the hopes that one of them can proofread the UTF8 stuff in GetOrigWindowSelection() in this patch.
(Assignee)

Comment 108

9 years ago
Created attachment 323502 [details] [diff] [review]
Changes addressing dmose's latest comments

Fixes per dmose's latest comments.  Continuing his sr+.

Waiting for comments from m-wada and/or jshin.
Attachment #313632 - Attachment is obsolete: true
Attachment #323502 - Flags: superreview+

Comment 109

9 years ago
I don't think m-wada usually does code reviewing, and a quick bugzilla query on jshin doesn't show much activity within the last year. 

m-wada, jshin: please comment here if you intend to look at the patch within any reasonable time. If not I think we should get this landed. Really looking forward to it!
Flags: wanted-thunderbird3.0a2?
Magnus, you are right. I have no experience of code review/patch creation at bugzilla.mozilla.org.
Comment on attachment 323502 [details] [diff] [review]
Changes addressing dmose's latest comments

smontagu's i18n fu is very strong.  Simon, all I'm really looking for here is review of the |if (requireMultipleWords)| clause of nsMsgComposeService::GetOrigWindowSelection()
Attachment #323502 - Flags: review?(smontagu)
Comment on attachment 323502 [details] [diff] [review]
Changes addressing dmose's latest comments

>+      for (; i != end; ++i)
>+      {
>+        const char c = *i;
>+        if (c == ' ' || (!UTF8traits::isASCII(c) && !UTF8traits::is2byte(c) && !UTF8traits::isInSeq(c)))
>+          break;
>+      }

Please document in code what you are doing here and why. It made no sense to me until I read back in the bug.

(In reply to comment #97)
> I'd appreciate comments from i18n folks on whether my technique for determining
> multiple words in the selection is okay (see
> nsMsgComposeService::GetOrigWindowSelection), which is basically remove leading
> and trailing spaces, and then look to see if the selection contains only
> non-space single and 2-byte UTF-8 chars.
> 

Single and 2-byte UTF-8 chars just means "unicode values < U+0800", so you could just test for that in the UTF-16 string without converting to UTF-8. But the test doesn't really do what you want. It will break in all Indic languages and with a bunch of commonly used punctuation, e.g. smart quotes. I think what you need to do here is use nsIWordBreaker::FindWord.
Attachment #323502 - Flags: review?(smontagu) → review-
(Assignee)

Comment 113

9 years ago
(In reply to comment #112)
> I think what you need to do here is use nsIWordBreaker::FindWord.

The only concrete instance I see of nsIWordBreaker in all of Mozilla code is nsSampleWordBreaker.  Is it okay to use that in production code?  Is there another concrete instance I'm missing?

Do all languages separate words by spaces?  That's true for latin-derived languages, but what about others?  Yes, there is punctuation to mark the end of a word, but before the next word there must be a space.  Could I simply iterate through a UTF-16 string looking for spaces?

Comment 114

9 years ago
(In reply to comment #113)
> Do all languages separate words by spaces?

No. Japanese, for instance, typically does not use interword space at all. Word breaks are between kana and kanji, or at word separator dots between words written just in katakana.
(In reply to comment #113)
> The only concrete instance I see of nsIWordBreaker in all of Mozilla code is
> nsSampleWordBreaker.  Is it okay to use that in production code?  Is there
> another concrete instance I'm missing?

I thought that we call into platform dependent routines (Uniscribe/Pango/Carbon), but I may be wrong. Roc probably knows better than I.
We use platform dependent calls combined with custom code to determine *line* break opportunities. *Word* breaking is used for word-based caret movement (e.g. ctrl-leftarrow).

You could use either here, really.
(Assignee)

Comment 117

9 years ago
Created attachment 324409 [details] [diff] [review]
More robust word detection

Here's a new version that uses nsILineBreaker to detect words.

It also fixes bit-rot on a couple of files.

Continuing dmose's sr+, but waiting for r+ by smontagu.
Attachment #323502 - Attachment is obsolete: true
Attachment #324409 - Flags: superreview+
Attachment #324409 - Flags: review?(smontagu)
Comment on attachment 324409 [details] [diff] [review]
More robust word detection

Looks good to me.
Attachment #324409 - Flags: review?(smontagu) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
I had a bit of trouble with the nsIMsgComposeParams.idl change not applying, couldn't see why, so I just applied it by hand as it was simple.

Checking in mailnews/compose/public/nsIMsgComposeParams.idl;
/cvsroot/mozilla/mailnews/compose/public/nsIMsgComposeParams.idl,v  <--  nsIMsgComposeParams.idl
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/compose/src/Makefile.in;
/cvsroot/mozilla/mailnews/compose/src/Makefile.in,v  <--  Makefile.in
new revision: 1.92; previous revision: 1.91
done
Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.568; previous revision: 1.567
done
Checking in mailnews/compose/src/nsMsgCompose.h;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.h,v  <--  nsMsgCompose.h
new revision: 1.120; previous revision: 1.119
done
Checking in mailnews/compose/src/nsMsgComposeParams.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeParams.cpp,v  <--  nsMsgComposeParams.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in mailnews/compose/src/nsMsgComposeParams.h;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeParams.h,v  <--  nsMsgComposeParams.h
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/compose/src/nsMsgComposeService.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeService.cpp,v  <--  nsMsgComposeService.cpp
new revision: 1.146; previous revision: 1.145
done
Checking in mailnews/compose/src/nsMsgComposeService.h;
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeService.h,v  <--  nsMsgComposeService.h
new revision: 1.26; previous revision: 1.25
done
Checking in mailnews/mailnews.js;
/cvsroot/mozilla/mailnews/mailnews.js,v  <--  mailnews.js
new revision: 3.315; previous revision: 3.314
done
Keywords: checkin-needed
Target Milestone: Future → mozilla1.9
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: Workarounds in comment #52
Flags: wanted-thunderbird3.0a2?

Comment 120

9 years ago
Wow, this is awesome!

Now, one question - for those of us who don't know the full dev process for TBird...

Will this new functionality now show up in the nightly builds? Will it show up in a coming 2.0.x release? Or will it be limited to the 3.0.x builds?

Much appreciation to Jeff and the others who made this happen!

Charles
(Assignee)

Comment 121

9 years ago
(In reply to comment #120)
> Will this new functionality now show up in the nightly builds? Will it show up
> in a coming 2.0.x release? Or will it be limited to the 3.0.x builds?

This is feature is only in 3.0 at the moment.  That doesn't prevent someone from backporting to 2.0.x, but it's very unlikely you're going to see any new features go in to a 2.0.x release.

So, it will show up in nightlies and will be in the 3.0a2 release.

Comment 122

9 years ago
I voted for this bug many years ago, now is fixed... Thank you Jeff :)
Jeff, can you file a spinoff bug as per comment 104 and link to it here?
(Assignee)

Comment 124

9 years ago
(In reply to comment #123)
> Jeff, can you file a spinoff bug as per comment 104 and link to it here?

Bug 439758.
Product: Core → MailNews Core

Updated

9 years ago
Depends on: 466330
Duplicate of this bug: 488642

Comment 126

8 years ago
Is there a way to disable this feature via a preference?

When reading an email, I often select sections while reading.  Then when I reply, I only have part of the message.  I know that I can un-select before replying, but I do not see me ever wanting this behavior.

Thanks.

Comment 127

8 years ago
(In reply to comment #126)
> Is there a way to disable this feature via a preference?

You have to set "mailnews.reply_quoting_selection" to false in about:config

Comment 128

8 years ago
Thank you, thank you, thank you, everyone: It's great to have this feature working in TB3!

Comment 129

8 years ago
I'm going to take that as "verified" :-)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 543152
Depends on: 543152

Comment 131

7 years ago
Is there the possibility to add a GUI preference, to turn this "feature" (or should I say annoyance) off, or alternative disable this again by default? Fiddling in the about:config is not very comfortable, and it took me hours to find the good key...

Updated

7 years ago
Depends on: 610376

Comment 132

5 years ago
Totally agree.  This is a very annoying "feature".  In fact, I didn't even realize that the selected text was what was being included in the reply for a few days.  It just seemed messed up.
You need to log in before you can comment on or make changes to this bug.