Review Request 117550: Show chat subject in group chat header and make it editable

[Martin Klapetek]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117550/#review55660
-----------------------------------------------------------


Generally looks good, couple issues below 


app/chat-window.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38735>

    if( -> if (
    
    (space after if)



app/chat-window.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38736>

    This should translatable string --> i18n(..), preferably use i18nc("comment for translators", "string")



app/chat-window.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38738>

    Given this, you can also have the slot without any argument, it will work.
    
    Generally slots must have equal or less number of args than the signals they are connected to.
    
    But it's no problem to leave it like this, just sayin' :)



config/appearance-config-tab.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38742>

    Note that David has a patch on RB that changes a lot of ChatView, I'd suggest to talk to him so you can figure out which patch should go in first



lib/adium-theme-header-info.h
<https://git.reviewboard.kde.org/r/117550/#comment38743>

    The passed string should be const & --> const QString &subject



lib/adium-theme-header-info.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38747>

    How about returning the chat name here if there's empty subject? That would prevent bugs in code I'll point more below...



lib/adium-theme-view.h
<https://git.reviewboard.kde.org/r/117550/#comment38745>

    Any reason this is not pointer? Given you're returning it as a pointer in headerInfo()



lib/adium-theme-view.cpp
<https://git.reviewboard.kde.org/r/117550/#comment38748>

    ...this code.



lib/chat-widget.h
<https://git.reviewboard.kde.org/r/117550/#comment38749>

    actorId -> is that the person who changed the subject? "actor" seems a bit...new :)


- Martin Klapetek


On April 13, 2014, 8:08 p.m., Leon Handreke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117550/
> -----------------------------------------------------------
> 
> (Updated April 13, 2014, 8:08 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-text-ui
> 
> 
> Description
> -------
> 
> Show chat subject in group chat header and make it editable
> 
> 
> Diffs
> -----
> 
>   app/chat-window.cpp 8608c0a61a148a9d601d7a34b3e5591eea4743db 
>   app/chat-window.h 72bbd1d4cfaece92d344979934873504dad8353a 
>   app/chatwindow.rc 3ad0c8f401f6a7e3b55ea877ce747cb79415e8ed 
>   app/main.cpp 0d882c1b94c698ff3b13a2a8e83760c19c7be13d 
>   config/appearance-config-tab.h ab0789a225f98ee96efb7e5a7d905b55f0c790f8 
>   config/appearance-config-tab.cpp 5f90c29120968e202e571d3350980691e98d5270 
>   lib/adium-theme-header-info.h 177d699c5e9aa9389c0b7ea8312ebfc04a8294dc 
>   lib/adium-theme-header-info.cpp 3dfa249fc13558822b4991c0881cbca70240f709 
>   lib/adium-theme-view.h 2628ba896f510f489b98b741ea951aec0289b981 
>   lib/adium-theme-view.cpp 72306a65d43a5c83a23ec0139d56071a9b068d77 
>   lib/chat-widget.h d9c4e6076bb7f47f4584f665b799f5db42bc418f 
>   lib/chat-widget.cpp 368274286a193f877cfbe7a4314fafbaa9b9e3c2 
>   logviewer/message-view.cpp ba69532390893bd0d16e65d84348f1373687effb 
> 
> Diff: https://git.reviewboard.kde.org/r/117550/diff/
> 
> 
> Testing
> -------
> 
> Group chats, normal chats. The button seems to be enabled only in the right circumstances and everything seems to be working well.
> 
> 
> Thanks,
> 
> Leon Handreke
> 
>

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