> 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.
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. - David ----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
