Review Request: Improve AddContactDialog

[Martin Klapetek]

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

Ship it!


Nicely done, thanks! Just few small nitpicks below..


KTp/Widgets/add-contact-dialog.cpp
<http://git.reviewboard.kde.org/r/105515/#comment12191>

    Star aligned to the right



KTp/Widgets/add-contact-dialog.cpp
<http://git.reviewboard.kde.org/r/105515/#comment12192>

    "...has been disconnected..." I'd say. Native speakers?



KTp/Widgets/add-contact-dialog.cpp
<http://git.reviewboard.kde.org/r/105515/#comment12193>

    This sounds very techie. Would "Failed to create new contact" work?



KTp/Widgets/add-contact-dialog.cpp
<http://git.reviewboard.kde.org/r/105515/#comment12194>

    Imho too technical as well. "Failed to retrieve contact's presence/status"? Although that's not entirely correct, but do we need to scare users with terms like "presence subscription"?


- Martin Klapetek


On July 11, 2012, 2:35 p.m., George Kiagiadakis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105515/
> -----------------------------------------------------------
> 
> (Updated July 11, 2012, 2:35 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Being annoyed by the fact that the presence applet shows the AddContactDialog as a modal dialog (making plasma unusable in the meantime), I decided to move the contact subscription logic inside the dialog and therefore eliminate code duplication between the applet and the CL as well as make it easy to use the dialog in a non-modal way.
> 
> This patch also adds the KTP_NO_EXPORT macro, which we should start using to hide private symbols and prepare the library for frameworks.
> 
> Shortlog:
> George Kiagiadakis (9):
>       AddContactDialog: Move function implementations inside the KTp namespace
>       ktp-export.h: Add KTP_NO_EXPORT macro
>       AddContactDialog: Move the contact creation logic from the presence applet & CL inside the dialog
>       AddContactDialog: Remove public accessors for account and screen name
>       AddContactsDialog: Use QComboBox::setModel() instead of manually populating it with entries from the model
>       AddContactDialog: Error out if the user did not specify a contact name
>       AddContactDialog: kDebug++
>       AddContactDialog: Use KMessageBox::sorry() instead of error() and enhance the strings
>       AddContactDialog: Attribute copyright to myself
> 
> 
> Diffs
> -----
> 
>   KTp/Widgets/add-contact-dialog.h cc65aa3f6a94884a83bd058e876cecc5e996fb85 
>   KTp/Widgets/add-contact-dialog.cpp dfbd9aac870a4d82953820fa4feb28e3af9d7f3d 
>   KTp/ktp-export.h 909c3ca9f1b8ddaed28c0fc15b2537da2a843760 
> 
> Diff: http://git.reviewboard.kde.org/r/105515/diff/
> 
> 
> Testing
> -------
> 
> The dialog still seems to work as expected
> 
> 
> Thanks,
> 
> George Kiagiadakis
> 
>

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