Review Request 116940: State-affinity and engaging now Playing from a custom status message fixes.

[James Smith]

> On March 26, 2014, 11:58 a.m., David Edmundson wrote:
> > Can you explain please.
> > "Fixes engage-nowPlaying-from-custom-status-message "
> > 
> > It really helps if I know what the bug we're trying to fix is and what's wrong with the existing code.

http://bugs.kde.org/show_bug.cgi?id=332082. The existing code is a) incompatible with the contact list when engaging a plugin from a custom status message and b) not fully taking advantage of the two boolean checks for active plugins. This code provides more predictable disk writes, dependable message selection in statusMessageStack(), and removes a piece of syncing code that's no longer useful.

The patch also contains a white-space fix for a comment in telepathy-module.cpp, and a white-space fix for telepathy-module.h


> On March 26, 2014, 11:58 a.m., David Edmundson wrote:
> > telepathy-module.cpp, line 161
> > <https://git.reviewboard.kde.org/r/116940/diff/2/?file=257308#file257308line161>
> >
> >     I don't understand this (and this is in the previous code)
> >     
> >     Why would we call setPresence when the requestedPresence has changed.

When the nowPlaying engine finishes playing or changes tracks sometimes the status message isn't properly cleared or updated. This is just another check to make certain the status message is kept current, for example when the presence applet changes presences.

On testing with the above changes, this code is now redundant.


> On March 26, 2014, 11:58 a.m., David Edmundson wrote:
> > telepathy-module.cpp, line 74
> > <https://git.reviewboard.kde.org/r/116940/diff/2/?file=257308#file257308line74>
> >
> >     We never write this. This is always going to be true.

I wanted a way to make the default plugin-set messages over user-set messages optional, so I put in a hidden config to match the behaviour of .8 . This is only true in the case where it hasn't been deliberately set to false in the config file.


> On March 26, 2014, 11:58 a.m., David Edmundson wrote:
> > telepathy-module.h, line 87
> > <https://git.reviewboard.kde.org/r/116940/diff/2/?file=257307#file257307line87>
> >
> >     This is also in the struct above. 
> >     
> >     Storing the same data twice is almost always going to end in things going wrong.

Separating was required for comparison purposes in statusMessageStack, because round-tripping through onRequestedPresenceChanged, m_lastUserPresence.statusMessage() wasn't consistent enough to use the user-set presence variable for anything useful. Also because with booleans for indicating active plugins it's more straightforward to properly control what is saved as a user presence. This allows to use user presences and status messages reliably in statusMessageStack(), and, also dump a piece of (spotted) redundant code in addition.


> On March 26, 2014, 11:58 a.m., David Edmundson wrote:
> > telepathy-module.cpp, line 142
> > <https://git.reviewboard.kde.org/r/116940/diff/2/?file=257308#file257308line142>
> >
> >     I don't understand why we're splitting status messages and statuses.
> >     You're not saving disk writes/anything as the next if statement is going to trigger anyway as the statusMessage in the struct will differ.
> >     You're now actually doubling it.

No, the variables are required separately to (for example) keep the status message returning to a custom message after nowPlaying terminates. Stuffing it into a new presence variable works just as well for saving and leave no changes for autoconnect, but adds 3 lines of code including a new global presence specifically for writing to disk.


- James


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


On March 28, 2014, 10:44 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated March 28, 2014, 10:44 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Bugs: 332082
>     http://bugs.kde.org/show_bug.cgi?id=332082
> 
> 
> Repository: ktp-kded-module
> 
> 
> Description
> -------
> 
> Fixes engage-nowPlaying-from-custom-status-message and lessens disk activity on presence change. Also simplifies logic used to keep telepathy-kded-module in a "fit" state during run-time.
> 
> 
> Diffs
> -----
> 
>   telepathy-module.h 2213cdf 
>   telepathy-module.cpp 030a0d9 
> 
> Diff: https://git.reviewboard.kde.org/r/116940/diff/
> 
> 
> Testing
> -------
> 
> Compile, run.
> 
> 
> Thanks,
> 
> James Smith
> 
>

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