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