Review Request: Add Global Presence drop down menu to the contact list

[Martin Klapetek]

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


Very nice, below are few mostly minor issues/discussion stuff, once sorted out, I'm happy with shipping it


dialogs/custom-presence-dialog.h
<http://git.reviewboard.kde.org/r/102805/#comment6284>

    Those two comments are a bit misleading as it adds the presences only to the model, which then saves them to the config



dialogs/custom-presence-dialog.h
<http://git.reviewboard.kde.org/r/102805/#comment6277>

    Whitespace



dialogs/custom-presence-dialog.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6278>

    Lots of whitespace leftovers in this method



dialogs/custom-presence-dialog.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6279>

    I found myself confused few times while closing this dialog as I expected that the new/selected presence would be selected as the current one. Should this also select the presence upon closing?



dialogs/custom-presence-dialog.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6280>

    Why leave out the 'Extended away' presence?



dialogs/custom-presence-dialog.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6281>

    Could this be potentially useful?



dialogs/custom-presence-dialog.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6282>

    The combobox entry uses 'Presences', we should use the same here instead of 'Status'



global-presence-chooser.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6283>

    This should have ... at the end



global-presence-chooser.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6269>

    This could probably be some more descriptive debug output :P



global-presence.h
<http://git.reviewboard.kde.org/r/102805/#comment6268>

    You can probably remove this...



kpresence.h
<http://git.reviewboard.kde.org/r/102805/#comment6270>

    Perhaps this could later be part of the common git repo?



main-widget.ui
<http://git.reviewboard.kde.org/r/102805/#comment6274>

    The combobox should have a maxWidth set, because if the contact list is opened with very long presence message, the window gets veeeeeeery wide. Don't forget text eliding. Perhaps the better place for this would be the GlobalPresenceChooser ctor.



presence-model.h
<http://git.reviewboard.kde.org/r/102805/#comment6271>

    Useless stuff



presence-model.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6272>

    I think this should go to the "shared" config ktelepathyrc



presence-model.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6273>

    I don't like the bold text on teh 'main' presences. Better solution would imho be to either leave all the text the same or use separators for groups, like:
    
    (O) Available
    -------------
    (Q) Away
    (Q) Went for food
    -------------
    (O) Busy
    (C) Not available



presence-model.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6275>

    You seem to forgot 'Not available'/'Extended Away'



presence-model.cpp
<http://git.reviewboard.kde.org/r/102805/#comment6276>

    You should check if the presence is not already in the list so we won't end up with duplicate presences


- Martin Klapetek


On Oct. 8, 2011, 1:46 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102805/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2011, 1:46 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> BOOM! Start of the global presence chooser. The UI may need some tweaks later, but it's already many commits long and I'd like to get this merged as soon as possible so everyone can start working on it and sorting out remaining parts together.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 1ff1c6b 
>   abstract-contact-delegate.h 5f73670 
>   abstract-contact-delegate.cpp 34e0d67 
>   dialogs/custom-presence-dialog.h PRE-CREATION 
>   dialogs/custom-presence-dialog.cpp PRE-CREATION 
>   global-presence-chooser.h PRE-CREATION 
>   global-presence-chooser.cpp PRE-CREATION 
>   global-presence.h PRE-CREATION 
>   global-presence.cpp PRE-CREATION 
>   kpresence.h PRE-CREATION 
>   kpresence.cpp PRE-CREATION 
>   main-widget.h b04cbd0 
>   main-widget.cpp 6a734e3 
>   main-widget.ui a84e682 
>   presence-model.h PRE-CREATION 
>   presence-model.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/102805/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

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