Closed Bug 23394 Opened 25 years ago Closed 16 years ago

Quote just the selected portion of a message during Reply

Categories

(MailNews Core :: Composition, enhancement, P3)

Product:
MailNews Core
Component:
Composition
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.9

People

(Reporter: John.planb, Assigned: beckley)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(1 file, 6 obsolete files)

More robust word detection
25.70 KB, patch
smontagu
: review+
beckley
: superreview+
Details | Diff | Splinter Review 
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.
Blocks: gnksa
Summary: Quote just the selected portion of a message → Quote just the selected portion of a message
cc: phil.  Phil, didn't you file bugs for a bunch of the gnksa items already?
Status: NEW → ASSIGNED
Target Milestone: M18
No Idea if we will do it, will see...could be nice...
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).
QA Contact: lchiang → pmock
<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 :-(.
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 ...
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
Looks like the helpwanted keyword got lost -- this should clearly be a
helpwanted bug.
Keywords: helpwanted
> 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).
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).
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).
*** Bug 89610 has been marked as a duplicate of this bug. ***
*** Bug 107553 has been marked as a duplicate of this bug. ***
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?
QA Contact: pmock → sheelar
*** Bug 69129 has been marked as a duplicate of this bug. ***
*** Bug 129650 has been marked as a duplicate of this bug. ***
*** Bug 131365 has been marked as a duplicate of this bug. ***
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
> 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.
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...
>> 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?
*** Bug 148669 has been marked as a duplicate of this bug. ***
QA Contact: sheelar → esther
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
Betsy: that would be a separate bug entirely.
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.
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.
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."
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
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.
> 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".
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.
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.
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
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?
*** Bug 193370 has been marked as a duplicate of this bug. ***
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.

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.
*** Bug 223094 has been marked as a duplicate of this bug. ***
*** Bug 223098 has been marked as a duplicate of this bug. ***
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.

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!
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?
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)
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
*** Bug 249267 has been marked as a duplicate of this bug. ***
*** Bug 257404 has been marked as a duplicate of this bug. ***
*** Bug 268106 has been marked as a duplicate of this bug. ***
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
*** Bug 272898 has been marked as a duplicate of this bug. ***
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
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
Whiteboard: Workarounds in comment #52
(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?
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
*** Bug 281400 has been marked as a duplicate of this bug. ***
Blocks: majorbugs
No longer blocks: majorbugs
*** Bug 299823 has been marked as a duplicate of this bug. ***
*** Bug 305549 has been marked as a duplicate of this bug. ***
(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.
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
*** Bug 312198 has been marked as a duplicate of this bug. ***
Is this EVER going to make into the core code???????
Yes. The new century has only just begun. :-P

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!
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.
(In reply to comment #64)
> http://quickquote.mozdev.org/installation.html

Install fails in Mozilla; seems only to work with Firefox/Thunderbird?
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.
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
*** Bug 326992 has been marked as a duplicate of this bug. ***
"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).
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/
I am not a programmer, but is this feature so hard to implement?
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)



*** 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
Blocks: eudora
*** Bug 361435 has been marked as a duplicate of this bug. ***
Blocks: 359227
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

This is essential.
This is essential.
Status: NEW → ASSIGNED
Assignee: nobody → beckley
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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)
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...
Thanks for implementing this important feature. Just looked at the patch superficially, but it looks very good (small and clean).
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.
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.

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.

While your tinkering, remember changing the IDL's UUID. ;-)
Attached patch New patch from comments (obsolete) — — Splinter Review 
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 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+
 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.

(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.
> 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.
> 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.
(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.

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*/).
> 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).
Attached patch Now with multi-word check (obsolete) — — Splinter Review 
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 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)
Keywords: helpwanted
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)
Attachment #307762 - Flags: superreview?(mscott)
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
Attachment #307762 - Flags: superreview?(dmose)
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-
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.
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+
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-
(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?

(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.
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+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: Workarounds in comment #52
Flags: wanted-thunderbird3.0a2?
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
(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.

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?
(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
Depends on: 466330
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.
(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
Thank you, thank you, thank you, everyone: It's great to have this feature working in TB3!
I'm going to take that as "verified" :-)
Status: RESOLVED → VERIFIED
Depends on: 543152
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...
Depends on: 610376
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.

Attachment

General

Creator:
Created:
Updated:
Size: