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

James Smith smithjd15 at gmail.com
Tue Apr 1 13:07:14 UTC 2014 


> 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.
> 
> James Smith wrote:
>     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.
> 
> David Edmundson wrote:
>     >On testing with the above changes, this code is now redundant.
>     
>     Well that does make me like this patch more  :)
>     Should we clean that up then?
> 
> David Edmundson wrote:
>     Oh it would have been caused by the 
>         if (m_globalPresence->currentPresence() != presenceThrottle()) {
>      
>     change as the timings would have been offset.
>     
>     I should have caught it in whichever review that was and not have allowed the previous hack.

I don't mind this code at all, esp. for it's behavior with the presence applet.


> 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.
> 
> David Edmundson wrote:
>     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)
>     
>     
>
> 
> James Smith wrote:
>     The ability to engage nowPlaying from the contact list which coincidentally with a little work can override the custom-set presence message (I'm personally in favor of warning the user to start from an empty status) is probably why the variable gets ahead of itself as part of a pair (presence - status message). When there is a user-set presence with status message the last-known user set status message is still required for when the nowPlaying plugin disengages, and so the variable is full. The variable is also required when competing to become the next visible status message. So, what we do is try and keep it separate until its paired with the presence and written to disk, and probably in the same go re-set as the visible status message, thus keeping it useable enough to perform status message replacement operations from its compares.
>     
>     if m_lastUserPresence.statusMessage().isEmpty() isn't true I can't properly compare for the next course of action, which may or may not include using m_lastUserPresence.statusMessage() in some role. m_lastUserPresence.statusMessage() can currently be overwritten somewhat unpredictably in onRequestedPresenceChanged().
>     
>     In the current solution, for instance, if presence.statusMessage() is not empty then bail before the presence is reset on-the-go before the user knows there's a visible status change. This code got around a bug which slightly hid a falling-over (with non-separate) presence + status message variables statusMessageStack(), due to comparison issues where there's a custom message, in addition to a plugin-supplied presence message. The current code may be more "correct", but the contact list still allows engaging from custom messages, meaning that variable isn't always verified properly filled, or the plugin engage logic falls over because the presence status message isn't empty despite not being the top contender for visible status message. Seen in addition to the ability for the presence status message to change from an external event, with a non-empty m_lastUserPresence.statusMessage() the compare broke with (in the same session) a new status message from nowPlaying which showed a problem where previously a isEmpty() on the m_lastUserPreesence.statusMessage() worked to keep the proper status message visible.
>     
>     Given status messages changes do necessarily occur with or without a change in presence, we don't know with certainty what the current status message is supposed to be, and what the user-set status message is supposed to be, or have any way to change between the two for the user. I think the bug is ultimately caused by the requirement to set a plugin-based status message when the user-set custom message must be also juggled in the switch to a plugin status message. When m_lastUserPresence.statusMessage() isn't empty the logic falls over trying to compare.
>     
>     I like the current .8 code because it works well with the presence applet. Most of these changes are required to allow the contactlist to work with a custom message, where instead I would just warn the user not to engage the nowPlaying from a custom status message, and make sure it turns on and off properly.
> 
> David Edmundson wrote:
>     Thanks for the reply, but I am still very confused.
>     
>     What is the situation in which m_lastUserPresence.statusMessage does not contain the last status message the user set. Surely if that happens it must be a bug.

It's very hard to pinpoint, but anywhere in onRequestedPresenceChanged() is suspect. We don't keep an accurate record of custom status messages to compare against plugin-originated status messages.


- James


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


On March 30, 2014, 3:15 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated March 30, 2014, 3:15 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.cpp 030a0d9 
>   telepathy-module.h 2213cdf 
> 
> 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/20140401/b55e0215/attachment-0001.html>

More information about the KDE-Telepathy mailing list