Review Request: Fix formatting plugin

[David Edmundson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106005/#review19141
-----------------------------------------------------------

Ship it!


Ooh, that's nice. I like how it will be easy to make this configurable.

Minor comments


filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15209>

    could you add a comment above explaining the regex in plain text
    
    i.e
    "matches "start of a string or whitespace, followed by %1 followed by either ...     the first argument will be replace by the tag '*','/' etc. "
    
    Regexs are so hard to read, it just looks like a cat ran over the numberpad pressing things.



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15206>

    QLatin1Char and single quotes
    
    Even though it makes very little actual difference given this is only run once, we'll get a Krazy warning on it.



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15204>

    Latin1Char



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15202>

    Latin1Char



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15203>

    Latin1Char



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15207>

    const &



filters/formatting/format-filter.cpp
<http://git.reviewboard.kde.org/r/106005/#comment15208>

    const & *2



filters/formatting/ktptextui_message_filter_formatting.desktop
<http://git.reviewboard.kde.org/r/106005/#comment15210>

    Not sure about the new name (though it's better).
    "Plain text formatting" doesn't imply it makes it non plain-text at the end.
    
    Ping me on IRC, lets brainstorm a few ideas / you can convince me this is best.



filters/formatting/ktptextui_message_filter_formatting.desktop
<http://git.reviewboard.kde.org/r/106005/#comment15211>

    I'd consider changing this :)
    
    (actually it might make sense to make all plugins shipped by KTp to have KTp as the author and email contact as we all can hack on them ?)
    We can go over that some other time.


- David Edmundson


On Sept. 19, 2012, 7:35 a.m., Daniele Elmo Domenichelli wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106005/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2012, 7:35 a.m.)
> 
> 
> Review request for Telepathy, David Edmundson and Lasath Fernando.
> 
> 
> Description
> -------
> 
> Handle '_' as underlined and '/' as italics, fix the regexp and replacement string and re-enable the formatting plugin
> 
> 
> Diffs
> -----
> 
>   tests/message-processor-basic-tests.h 5ffcc92864820d9fd8c535cd4e249cb31310011b 
>   tests/message-processor-basic-tests.cpp a404908152ef9e21eb7e98cb472775f78c8a4162 
>   filters/CMakeLists.txt db5cffeb5918c6273086905b1691a2ed15a58cbc 
>   filters/formatting/format-filter.h 4980aff8251a91b2e23de2a7d8257ed25b5d1322 
>   filters/formatting/format-filter.cpp 75157fa4b7cd70ad0a1bfdfe7fc8886c37a75ba1 
>   filters/formatting/ktptextui_message_filter_formatting.desktop d43edf8722e18fb104e774ac158c76732e9173a4 
> 
> Diff: http://git.reviewboard.kde.org/r/106005/diff/
> 
> 
> Testing
> -------
> 
> Unit tests included and a few more visual test
> 
> 
> Thanks,
> 
> Daniele Elmo Domenichelli
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120919/d1a8b5ec/attachment-0001.html>