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