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

[David Edmundson]

> 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.
> 
> James Smith wrote:
>     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.

OK, could you document this in the code.


> 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.
> 
> James Smith wrote:
>     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.
> 
> James Smith wrote:
>     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.

What confuses me is in the original design this bug shouldn't exist. m_lastUserPresence should contain the last *user set presence* which is what we should return to after all plugins terminate. 

Clearly if m_lastUserPresence doesn't contain the last user presence something is very wrong. Anything else is either a bodge _OR_ the original design is flawed and we need to move to a new design which includes renaming this variable (and/or variable type to Tp::ConnectionPresenceType)


- David


-----------------------------------------------------------
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/20140329/b1ba034e/attachment.html>