Review Request 113066: Refactor Now playing plugin

[David Edmundson]

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


Looks pretty good to me. 

first topic needs discussion, once we agree on something consider this a ship it.

(would be good if someone else has a look too, it's a big change)


config/telepathy-kded-config.ui
<http://git.reviewboard.kde.org/r/113066/#comment30183>

    This is an i18n change and we're in string freeze.
    
    We can either:
     - not push patch this in 0.7
     - push it without this one line change and do that in a new commit in master
     - email i18n teams and beg forgiveness 
     - not have this string, restore now playing if it was running
     - or simply don't restore it at login at all, it's quite easy to activate.



telepathy-mpris.cpp
<http://git.reviewboard.kde.org/r/113066/#comment30184>

    super pedantic nitpick
    
    reenable -> re-enable
    
    change->changed


- David Edmundson


On Oct. 2, 2013, 7:47 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113066/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2013, 7:47 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Bugs: 307582 and 313126
>     http://bugs.kde.org/show_bug.cgi?id=307582
>     http://bugs.kde.org/show_bug.cgi?id=313126
> 
> 
> Repository: ktp-kded-module
> 
> 
> Description
> -------
> 
> This patch contains following changes:
> 1. disconnect for PropertiesChanges when knownPlayer is reset, the disappered service will not automatically disconnect from dbus.
> 2. change the "Enable" option to "Enable On Login" (bug 307582) and hence we can simplifies the logic (no more presenceActivated)
> 3. Changes now playing status at contact list will be remembered only for current login.
> 4. Fix the wrong behavior of handling PropertiesChanges (invalidate properties is not considered).
> 5. use requestedPresence to evaluate the current status (part of bug 313126)
> 6. since it changes to "Enable On Login", always enable the nowplaying text lineedit in kcm.
> 
> Current logic:
> 1. If the enabled option in config changes, set the plugin enable state (this must be caused by user change in kcm).
> 2. receive onActivate / onDeactivate from contact list, set the plugin enable state.
> 3. when enable state change to true, start detecting player. (activatePlugin(bool))
> 4. when enable state change to false, unwatch all player, reset all other state. (activatePlugin(bool))
> 5. when new player appears, request playbackStatus and metadata by GetAll, and then watch on the PropertiesChange for this player.
> 
> Some improvement:
> 1. not always listening on expensive serviceOwnerChanged.
> 2. be able to enable nowplaying on login.
> 3. fix leak connection of dbus signal.
> 4. code should now be clearer instead of having too much boolean check.
> 
> 
> Diffs
> -----
> 
>   config/telepathy-kded-config.cpp 953a637 
>   config/telepathy-kded-config.ui 09729e7 
>   telepathy-mpris.h e3378d4 
>   telepathy-mpris.cpp 6cd3e20 
> 
> Diff: http://git.reviewboard.kde.org/r/113066/diff/
> 
> 
> Testing
> -------
> 
> Everything seems to work as expected.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

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