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 641845 - Add default expansion variables to templates plugin
Add default expansion variables to templates plugin
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Plugins
unspecified
Other All
: Normal enhancement
: ---
Assigned To: evolution-plugin-maintainers
Evolution QA team
Depends on:
Blocks:
 
 
Reported: 2011-02-08 17:04 UTC by Fabian Fagerholm
Modified: 2011-09-19 20:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Implementation of expansion variables (12.09 KB, patch)
2011-03-31 13:06 UTC, Dan Vrátil
none Details | Review
Implementation of expansion variables + popup menu optimization (21.75 KB, application/octet-stream)
2011-04-18 16:19 UTC, Dan Vrátil
  Details
Complete patch (22.75 KB, patch)
2011-04-18 16:21 UTC, Dan Vrátil
needs-work Details | Review
Fixed patch with all issues mentioned by Milan resolved. (23.70 KB, patch)
2011-04-19 15:51 UTC, Dan Vrátil
none Details | Review
Fixed patch (23.24 KB, patch)
2011-04-20 16:28 UTC, Dan Vrátil
none Details | Review
test template (470 bytes, text/plain)
2011-04-22 15:25 UTC, Milan Crha
  Details
patch on the patch (4.47 KB, text/plain)
2011-04-22 15:36 UTC, Milan Crha
  Details
Fixed patch (24.82 KB, patch)
2011-04-26 13:52 UTC, Dan Vrátil
committed Details | Review

Description Fabian Fagerholm 2011-02-08 17:04:08 UTC
Currently when replying to a message using a template, the following two things happen automatically:

 * The From address in the original message is used as To address in the reply.
 * The Subject in the original message is used as Subject in the reply.

It would be very useful to be able to access parts of the original message in the template, both in the headers of the template and in the body. I would suggest something like a $ORIG[] dictionary with a case-insensitive string key to access each header in the message? $ORIG['body'] would be the entire body.

This would allow a template such as:

----------8<----------
To: $ORIG['from']
Cc: $ORIG['cc']
Subject: Meeting confirmed (Re: $ORIG['subject'])

I have received your email and confirm the meeting as suggested.

Kind regards,
N. N.

-------- Original Message --------
From: $ORIG['from']
To: $ORIG['to']
Subject: $ORIG['subject']
Date: ORIG['date']

$ORIG['body']
----------8<----------

(Of course, the To, Cc, and Subject above are GUI fields, and the rest is the text field where the message is typed.)

Expansion would occur when the template-based message is displayed, so you could still make final edits before sending.

$ORIG['body_quoted'] could perhaps be the body as it would appear if using a normal reply (with > in front of every line).

Or maybe it should be $HEADER['key'], $BODY, $BODY_QUOTED, or something. In any case, this would really make the template plugin more useful.
Comment 1 Milan Crha 2011-02-28 17:13:41 UTC
Thanks for a bug report. I like the idea. One question, though, what body would be returned when replying to a message with text/plain and text/html parts, or even with text/calendar part? These bodies might be slightly complicated.
Comment 2 Dan Vrátil 2011-03-31 13:06:28 UTC
Created attachment 184775 [details] [review]
Implementation of expansion variables

This patch implements support for $ORIG[header_name] expansion variables. You can use any header you want + there's $ORIG[body] to access body of the original message.

This patch assumes, that most sane email clients add plain-text version of text/html or text/calendar content. 

When the template is text/html, this patch first looks for text/html part of the message you are replying to. When no text/html part is available, it takes text/plain and converts it to text/html. When template is text/plain, it tries to use text/plain part of the message you are replying to. If any of the logic about fails (e.g. no part with a sane content-type is found), the $ORIG[body] is only removed and it's not replaces by anything (because such bogus message is considered to be broken).
All other parts of the template are converted to attachments. Obviously no attachments from the source message are taken.

Finally, when no replacement for a variable is found, the variable is NOT removed (except for $ORIG[body]), but it's left there so that you see that you have to fill it by hand. This can happen when using something like $ORIG[reply-to] or some more obscure header variables, but should not occur with standard from,to,subject or date.

A short description of the possibilities was added to the plugin description.

PS: is there a way how to manage the template messages? During the testing I created like 20 templates but I don't see them anywhere else but in the email-list popup menu :) Otherwise I was thinking about adding a UI for templates management into plugin configuration page. I think removing files from ~/.local/share/evolution is not a very user-friendly way.
Comment 3 Milan Crha 2011-04-01 06:27:24 UTC
(In reply to comment #2)
> PS: is there a way how to manage the template messages? During the testing I
> created like 20 templates but I don't see them anywhere else but in the
> email-list popup menu :) Otherwise I was thinking about adding a UI for
> templates management into plugin configuration page. I think removing files
> from ~/.local/share/evolution is not a very user-friendly way.

There is no apart of On This Computer/Templates folder in the folder tree.
Comment 4 Dan Vrátil 2011-04-18 16:19:04 UTC
Created attachment 186211 [details]
Implementation of expansion variables + popup menu optimization

The are just minor changes in the implementation of expansion variables from previous patch. The major change, as mcrha suggested, is optimization of the 'Templates' submenu in message popup menu. The old way was that every change of message selection caused the plugin to throw away the old submenu, go through the 'Templates' folder, create the submenu again and then display the popup menu. Now the submenu is cached and plugin is watching the 'Templates' folder and all it's subfolders for changes. When a change occurs (new template or folder is created or removed), only then the plugin throws the menu away and generates a new one.
Comment 5 Dan Vrátil 2011-04-18 16:21:41 UTC
Created attachment 186212 [details] [review]
Complete patch

Sorry, I forgot to add org-gnome-templates.eplug.xml to patch, it contains very short explanations about usage of the expansion variables.
Comment 6 Milan Crha 2011-04-19 12:54:46 UTC
No need to pass GString ** into replace_template_variable, its value is inside the object, so it can be just a simple pointer. In that function, you've also minor coding style issues (two), and instead on that 'return', I would use 'break'. Of course, modification of the 'template' variable can realloc the inner str, which is stored in t_str, thus it can, in that while, use already freed memory - theoretically. I would copy&paste&modify the replace function from the backup/restore plugin, as that one is well-tested.

I see similar coding style issue in lower functions too, basically repeating. There is used code like:
   if (x) {
   } else {
   }

   while (y) {
   }

Not every email address can have a name, so when you are composing an address, please use camel_internet_address_format_address() which takes care of relevant cases. Seeing that, I would use a function and check for all To/CC/BCC, only to have the set complete.

In fill_template, after the check of "if (CAMEL_IS_MULTIPART (dw))", please add there a variable and cast it to multipart only once, not that many times below. Also test if 'ct' isn't NULL. It sometimes can be (I think).

After /* Get body of template part */, you can write_to_stream and decode_to_stream, I believe the later is better here, isn't it? Also, after that call, do just camel_stream_flush and template_body = g_string_new_len () from a byte array camel_stream_mem_get_byte_array() value, as this is already in memory, and you can just make it a null-terminated string.

Please initialize 'header' variable lower in the code, it seemed to me as used uninitialized on the first look.

In update_actions_cb, I would initialize variables only when plugin is enabled. I also noticed that the second initialization on 'length' variable is unused at the end of the function.

On menu rebuild, any previously added signal connection should be disconnected, on all folders, to avoid multiple callings on the same object (like the g_signal_connect in the build_template_menus_recurse() function.

gtk_action_group_list_actions() is returning newly allocated list, so you are leaking this one. How about inner actions in the list I'm not sure from its documentation.

It looks good apart of the above, even I didn't run it yet.
Comment 7 Dan Vrátil 2011-04-19 15:51:36 UTC
Created attachment 186295 [details] [review]
Fixed patch with all issues mentioned by Milan resolved.
Comment 8 Milan Crha 2011-04-20 16:14:08 UTC
Thanks, the patch looks quite good, I've only the last two things to change in it before I'll commit and actually test it:

a) I would like to change replace_email_addresses and replace_template_variable,
both might return void and take, as the first parameter GString, which will be changes inside the functions; it'll avoid that unnecessary ts = gstring; gstring = replace...; g_string_free (ts);

b) cope with the message body only when you are going to replace "$ORIG[body]", aka avoid all the relevant stuff for cases where template doesn't have this variable used
Comment 9 Dan Vrátil 2011-04-20 16:28:28 UTC
Created attachment 186372 [details] [review]
Fixed patch
Comment 10 Milan Crha 2011-04-22 15:25:46 UTC
Created attachment 186484 [details]
test template

This as an mbox containing a testing template. If you use it you may end with fully populated variables (note of case insensitive search required, as also header names can be case insensitive). I do not know how doable it can be for To/CC headers, but the subject of the template should be checked for variables too (and it may survive). You may "just" replace 'strstr' in the replace function to do a case insensitive search, which you may write your own, probably (I'm not aware of such function in glib), involving g_ascii_tolower or similar function.
Comment 11 Milan Crha 2011-04-22 15:36:25 UTC
Created attachment 186486 [details]
patch on the patch

patch on your patch, can be applied by:
  a) name your patch as evofix.patch
  b) invoke: $ patch -p1 <$THIS_ATTACHMENT

it adds more-or-less minor fixes to your otherwise great menu items caching mechanism. Notice two things:
a) store notifies you about each local folder, not only about templates, thus it's better to check the folder is under Templates at all
b) g_signal_handlers_block_by_func tests for both function and its data, and as you passed NULL for data, then it didn't remove any callback

Please make sure the test template will be changed properly and then this can go in sources. As I said, I like the menu items caching much.
Comment 12 Dan Vrátil 2011-04-26 13:52:52 UTC
Created attachment 186654 [details] [review]
Fixed patch

This patch contains applied patch from Milan, fixes the replace_template_variable() to be case insensitive (using native strcasestr when available or g_ascii_strdown+strstr) and allows Subject field to contain any $ORIG[] variable, not just $orig[subject]. Doing this for To/BCC/... fields would be possible too, but it would just make the code more complicated and I don't think it's worth the effort.
Comment 13 Milan Crha 2011-04-27 15:12:42 UTC
Thanks for the final update. I realized a leak at:
==30143==    by 0x7605DA2: camel_mime_part_new (camel-mime-part.c:940)
==30143==    by 0x225E27A6: fill_template (templates.c:680)
==30143==    by 0x225E2BF6: create_new_message (templates.c:751)
which I fixed.

Created commit 5fcaadb in evo master (3.1.1+)
Comment 14 PierreP 2011-09-19 20:59:49 UTC
Why make it simple if you can make it complicate ???
(As we say in French, pourquoi faire simple quand on peut faire compliqué ?).

For the sake of simpleness, I suggest the following :
 1 - Just enrich the "pattern" stuff so that it admits such keys as $ORIG[sender], $ORIG[quoted contents],$ORIG[nonquotedcontennts],$ORIG[cc], $ORIG[ccj],$MYACCOUNT[name],$MYACCOUNT[defaultsignature], $MYACCOUNT[<signature name>]...
 2 - of course, in contexts other than a reply/forward, $ORIG[*] generates a null,
 3 - in Configuration, allow define a "default reply template", a "defaultforward template" etc.,
 4 - let the rest work more or less like it now does, smartly using default options and the possibility to superseed them,
 5 - let the syntax be case sensitive or not, I don't mind.

Thie would make a part of Evolution more simple, more powerful and more easy to use and understand.

Cheerfully.