GNOME Bugzilla – Bug 680026
Text highlight module
Last modified: 2013-09-13 01:06:06 UTC
Created attachment 218924 [details] [review] Patch This patch adds support for automatic text highlighting of messages and attachments. It supports over 50 programming languages and their mime types. The module automatically registers itself as a parser for any mime type it supports (but "outsources" the parsing to the in-build text/plain parser). This way we get parsed even C source files, etc. The module automatically registers itself as a formatter for any mime type it supports as well and uses 'highlight' utility to perform the actual highlighting. Finally, the module also adds "Format as..." submenu into the preview pane and mail browser context menu, so that use can have to part highlighted by any format he chooses (and we support). Can be useful in case of mis-detection or when there is for instance a piece of C code in the plain text email and user would like to see the code highlighted. See http://www.progdan.cz/2012/06/evolution-gets-a-new-e-mail-formatter/ for some description of what the patch does in general.
so that use can have to part highlighted by any format => so that user can have the part highlighted by any formatter (I've been probably working too hard today :P)
a) languages.h should be added into Makefile.am too, otherwise it's unknown when building tarball. b) typo LANGAGES_H c) does it make sense to use mnemonics when they clash in the file already? d) you might include config.h in the file as the first thing, and only after that any other includes; the languages.c includes "languages.h" first e) I would add some checking for soup_uri not being NULL in reformat(); similarly check in update_actions() for uri != NULL f) webkit_dom_document_get_document_uri() suggests it's returning newly allocated string, make sure you'll free it, in update_actions() g) get_default_font() is not in correct coding style, same as in languages.h are missing spaces before * (not gchar*, but gchar *) h) in get_default_font(), the g_settings_new() result is not unreffed i) 0x7a7a7a is not a color user has set in preferences j) you might not need GSettings in emfe_text_highlight_format(), it's also not always freed, by the way, use EShellSettings and its "mail-use-custom-fonts" property. EMailShellSettings provides also "mail-font-monospace". It'll be good to change this in EMailDisplay too. k) camel_content_type_simple() returns newly allocated string l) I'm slightly confused from this line: camel_stream_flush (read, cancellable, NULL); it might make more sense when it's swapped with its previous line, but on that current place I'm wondering what it does. m) in emfe_text_highlight_mime_types() is missing space in call of get_mime_types(); n) does it make sense to name this _("Patch")/_("Format part as a patch") ? I think it would be better to remove all the get_display_name() and get_description() now, and if you have it planned after this will land, then OK o) do you recognize what formatter is currently used? It would be nice to see a check mark beside the action in the Format as menu on the one being currently used for formatting p) text/html messages are formatted with the highlighter, in raw HTML, instead of text/html formatter which passes data to webkit q) when I try test-inner-level2 message, then I see webkit window flashing, 100% CPU usage and below backtrace, which shows basically nothing useful. Reverting this patch makes the message shown as expected.
+ Trace 230533
Created attachment 219227 [details] [review] Fixed patch (In reply to comment #2) > a) languages.h should be added into Makefile.am too, otherwise it's unknown > when building tarball. Fixed > b) typo LANGAGES_H Fixed > c) does it make sense to use mnemonics when they clash in the file already? It definitely does, you can still jump between for example "Java" and "Java Script" by repeatedly pressing "J" - more comfortable then having to press down arrow X times to even get there. > d) you might include config.h in the file as the first thing, and only after > that any other includes; the languages.c includes "languages.h" first Fixed > e) I would add some checking for soup_uri not being NULL in reformat(); > similarly check in update_actions() for uri != NULL Fixed > f) webkit_dom_document_get_document_uri() suggests it's returning newly > allocated string, make sure you'll free it, in update_actions() Fixed > g) get_default_font() is not in correct coding style, same as in languages.h > are missing spaces before * (not gchar*, but gchar *) Fixed > h) in get_default_font(), the g_settings_new() result is not unreffed Fixed > i) 0x7a7a7a is not a color user has set in preferences Set to less-confusing 0. We don't care about color quoted text in text-highlight... > j) you might not need GSettings in emfe_text_highlight_format(), it's also > not always freed, by the way, use EShellSettings and its > "mail-use-custom-fonts" property. EMailShellSettings provides also > "mail-font-monospace". It'll be good to change this in EMailDisplay too. Fixed > k) camel_content_type_simple() returns newly allocated string Fixed > l) I'm slightly confused from this line: > camel_stream_flush (read, cancellable, NULL); > it might make more sense when it's swapped with its previous line, but > on that current place I'm wondering what it does. Removed, I probably forgot to remove it after some experiments > m) in emfe_text_highlight_mime_types() is missing space in call of > get_mime_types(); Fixed > > n) does it make sense to name this _("Patch")/_("Format part as a patch") ? > I think it would be better to remove all the get_display_name() and > get_description() now, and if you have it planned after this will land, > then OK Renamed, but I'll remove this feature completely after committing this. > > o) do you recognize what formatter is currently used? It would be nice > to see a check mark beside the action in the Format as menu on the one > being currently used for formatting Done & done > > p) text/html messages are formatted with the highlighter, in raw HTML, > instead of text/html formatter which passes data to webkit Fixed - text-highlight is not registered as parser of formatter of text/html anymore, but you can still force HTML highlighting from the context menu. > > q) when I try test-inner-level2 message, then I see webkit window flashing, > 100% CPU usage and below backtrace, which shows basically nothing useful. > Reverting this patch makes the message shown as expected. Can't reproduce on the same message and the backtrace is of no use. If you can still reproduce the issue locally, please try to get some better backtrace :-) PS: Could you please use the review tool in Bugzilla next time? For such big patches it's sometimes difficult to find which piece of code you refer to (like k) for example)
aa) e-mail-display-popup-text-highlight.h is missing in the Makefile.am (I didn't notice before). ab) extra spaces in string "Syntax highlighting of mail parts ", but if you'll remove it, then it doesn't matter. ac) might be nice to add separator before "Format as" menu. ad) I've text/plain message, I select the frame, I right-click and choose other type to use, like C# or HTML, then nothing happens. Then I right-click, message preview flashes on popup menu show, and eventually redraws with set formatting. Then I hide popup menu by clicking elsewhere (not choosing any option from "Format as"). Then I right click again, and the view flashes again, making view formatted back in text/plain mode. I can repeat this show/hide popup menu to switch between text/plain and selected formatting. I think you can fix at least aa) and ad) and then commit. We can deal with more issues (if any) when it's in sources. With respect of review tool, it's hard to read patches there for me, though I understand your request.
ae) text/plain message, like any from evolution-hackers list (as example, the last from dwmw2, or any recent from me), doesn't highlight quoted text, neither links, nor email addresses.
Created attachment 219585 [details] [review] Fixed patch (In reply to comment #4) > aa) e-mail-display-popup-text-highlight.h is missing in the Makefile.am (I > didn't notice before). Fixed > > ab) extra spaces in string "Syntax highlighting of mail parts ", but if > you'll remove it, then it doesn't matter. Fixed anyway > > ac) might be nice to add separator before "Format as" menu. Good idea, it looks much better now > > ad) I've text/plain message, I select the frame, I right-click and choose > other type to use, like C# or HTML, then nothing happens. Then I right- > click, message preview flashes on popup menu show, and eventually redraws > with set formatting. Then I hide popup menu by clicking elsewhere (not > choosing any option from "Format as"). Then I right click again, and the > view flashes again, making view formatted back in text/plain mode. I can > repeat this show/hide popup menu to switch between text/plain and selected > formatting. Fixed. I can't just reproduce your "flashing", for me this switch happens immediatelly without any glitches. Could it be related to your WebKitGtk+ version and/or graphic drivers? > ae) text/plain message, like any from evolution-hackers list (as example, the > last from dwmw2, or any recent from me), doesn't highlight quoted text, > neither links, nor email addresses. Fixed. When using "txt" syntax (e.g. Format as -> Plain Text) text-highlight automatically fallbacks to the native text/plain formatter instead of using the highlight utility.
Thanks for the update. This looks better, even there are still one corner case. Please commit and I'll file a new bug report for that I noticed.
Comment on attachment 219585 [details] [review] Fixed patch Committed to master as 8663b5 http://git.gnome.org/browse/evolution/commit/?id=8663b54cae296883452850f3baad6d4cbc92e220