----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/105701/#review16276 -----------------------------------------------------------
Great to see you managed the reviewboard at last! :) Nice patch, couple comments and hints below, but otherwise it looks good! kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12787> In c++ we don't use the class name to call the method, so it's ok to just call 'load();' Also you should specify what you're actually loading, so rename it to something like 'loadSettings()' or 'init()'. kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12788> Watch out for whitespace at the end of lines. kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12789> We usually put these comments in the header files. Also it's perfectly fine to just state what the method does, so you don't have to say "Implementation of default..." ;) kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12790> Whitespace kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12791> Don't use the class when calling the methods. kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12792> KGlobalSettings::generalFont() already returns QFont, so you don't have to construct it again (just remove the QFont( )). kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12793> Same as above kde-telepathy <http://git.reviewboard.kde.org/r/105701/#comment12794> Whitespace - Martin Klapetek On July 23, 2012, 9:28 p.m., Nick Lou wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/105701/ > ----------------------------------------------------------- > > (Updated July 23, 2012, 9:28 p.m.) > > > Review request for Telepathy. > > > Description > ------- > > Fix for the Bug 295106 (The reset button in kcm_ktp_chat_apparence is not > working). > The same problem was with the default button, fixed that too. > > > This addresses bug 295106. > http://bugs.kde.org/show_bug.cgi?id=295106 > > > Diffs > ----- > > kde-telepathy 2e0ed61 > kde-telepathy 876d0ed > > Diff: http://git.reviewboard.kde.org/r/105701/diff/ > > > Testing > ------- > > On theme config settings: > 1)Applying a theme. > 2)Pressing default button //action: setting options to default values. > 3)Pressing reset button //action:setting options to applied values > 4)Repeating this procedure several times, trying different theme combinations > before and after applying changes each time. > > > Thanks, > > Nick Lou > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
