After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 681279 - [regression] Reply on selection doesn't work
[regression] Reply on selection doesn't work
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Mailer
3.6.x (obsolete)
Other Linux
: Normal normal
: ---
Assigned To: evolution-mail-maintainers
Evolution QA team
evolution[webkit] evolution[formatter]
Depends on:
Blocks:
 
 
Reported: 2012-08-06 06:54 UTC by Milan Crha
Modified: 2018-03-29 12:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix (3.27 KB, patch)
2012-08-07 09:40 UTC, Dan Vrátil
committed Details | Review
evo patch (664 bytes, patch)
2012-09-07 13:38 UTC, Milan Crha
rejected Details | Review
proposed evo patch (2.92 KB, patch)
2012-09-12 10:26 UTC, Milan Crha
committed Details | Review

Description Milan Crha 2012-08-06 06:54:24 UTC
There was a useful feature on replies, if there was a selected text in the message preview, then only that selection was used for a reply quote. This used to work for ages, but it is not working with webkit now.

Steps:
a) select text in message body (beware of "empty selection")
b) hit Ctrl+R

Only the selected text should be used in the quoted part.
Comment 1 Dan Vrátil 2012-08-07 09:40:02 UTC
Created attachment 220521 [details] [review]
Fix

Interestingly, this must have been broken for some time now. The current code for obtaining selection from webview did not support iframes and because we stash everything into iframes, the code always failed, returned nothing and so we fell back to full message.

This patch changes the code so that it recursively traverses all iframes within the main document and returns the selected string as soon as it finds the correct iframe.
Comment 2 Milan Crha 2012-08-08 08:08:40 UTC
Works fine, it also fixed one runtime warning. Please commit. Thanks.
Comment 3 Dan Vrátil 2012-08-08 11:49:15 UTC
Comment on attachment 220521 [details] [review]
Fix


Committed to git master as 83c97e5

http://git.gnome.org/browse/evolution/commit/?id=83c97e53c0be962d2db12d09b6e1a8bf91fcd93e
Comment 4 Milan Crha 2012-09-07 06:10:42 UTC
I'm reopening this. Currently, if I selected a text in a message preview and press Ctrl+R/Ctrl+L to reply to it, then I end up with a composer which contains only the line about the reply [1], but not the selected text.

[1] On Thu, 2012-09-06 at 12:14 -0400, Someone wrote:
Comment 5 Milan Crha 2012-09-07 13:38:12 UTC
Created attachment 223763 [details] [review]
evo patch

for evolution;

Prefer-plain can hide text parts, like text/html when it's set to only show text/plain, thus this makes sure all text parts are shown when doing a quote (the selected text is quoted as text/html message). I didn't see in parser how to check the formatter is EMailFormatterQuote, thus I done it this way.
Comment 6 Milan Crha 2012-09-07 13:40:25 UTC
Created commit c9d0ac7 in evo master (3.5.92+)
Comment 7 Milan Crha 2012-09-11 10:03:28 UTC
Hrm, my commit from comment #5 causes replies to multipart/alternative contain both text/plain and text/html parts in quotation.
Comment 8 Milan Crha 2012-09-11 15:36:04 UTC
I reverted my patch from comment #5 in commit 9998479 (3.5.92+)
Comment 9 Milan Crha 2012-09-11 16:01:02 UTC
Another problem is that the result of the Reply is used for message preview, which makes any tweaking for quotation slightly useless.

I do not like the idea of such workarounds like I did in comment #5 (it just seemed like an easy way before the release), the correct way would be to set an intent to the EMailParser, thus extensions can react on it (in case of prefer-plain do not touch the result). Of course, the part list should be freed immediately after the reply is quoted, thus it's not reused for preview panels.
Comment 10 Dan Vrátil 2012-09-11 16:20:38 UTC
(In reply to comment #9)
> the correct way would be to set an intent to the EMailParser, thus extensions 
> can react on it (in case of prefer-plain do not touch the result). 

The idea of the new parser is that it is unaware of how the parts will be used and that it produces the same list of parts every time. Any differences in the output presented to user should be handled in formatter.

> Of course, the part list should be freed immediately after the reply is quoted, 
> thus it's not reused for preview panels.

You still could get into problems with concurrency.
Comment 11 Milan Crha 2012-09-11 16:35:23 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > the correct way would be to set an intent to the EMailParser, thus extensions 
> > can react on it (in case of prefer-plain do not touch the result). 
> 
> The idea of the new parser is that it is unaware of how the parts will be used
> and that it produces the same list of parts every time. Any differences in the
> output presented to user should be handled in formatter.

Yes, but it obviously doesn't work, sadly.

> > Of course, the part list should be freed immediately after the reply is quoted, 
> > thus it's not reused for preview panels.
> 
> You still could get into problems with concurrency.

Maybe, but my steps were pretty simple:
a) no preview, just hit Ctrl+R
b) when the message is shown, cancel its sending
c) double click the message, thus the preview is filled for the first time

I did it before I reverted my change to "show all text/* parts" and I saw all text parts in the preview. When I close evolution and open preview first, then it shows what is expected.
Comment 12 Milan Crha 2012-09-12 10:26:44 UTC
Created attachment 224091 [details] [review]
proposed evo patch

for evolution;

Trying to recover text/html part if it's the only one in this lever of parts.
Comment 13 Dan Vrátil 2012-09-12 10:37:20 UTC
Review of attachment 224091 [details] [review]:

Looks good, please commit.
Comment 14 Milan Crha 2012-09-12 10:42:23 UTC
Thanks. Created commit ea33b6c in evo master (3.5.92+)
Comment 15 Milan Crha 2013-01-29 13:00:36 UTC
A little regression after this changed logged at bug #692775
Comment 16 Milan Crha 2018-03-29 12:44:57 UTC
And one more regression, reported only now here:
https://bugzilla.redhat.com/show_bug.cgi?id=1560567#c18