----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124643/#review83503 -----------------------------------------------------------
I did just quick look at code, nothing deeply! Problems are marked as reviewboard issues with comments. Please fix them. plugins/CMakeLists.txt (line 8) <https://git.reviewboard.kde.org/r/124643/#comment57737> It is needed to move this line? plugins/CMakeLists.txt (line 29) <https://git.reviewboard.kde.org/r/124643/#comment57738> Same here... plugins/history/chathistoryplugin.h (line 22) <https://git.reviewboard.kde.org/r/124643/#comment57727> No, you cannot except something like there will be only one instance of Kopete::Plugin class. plugins/history/chathistoryplugin.h (line 35) <https://git.reviewboard.kde.org/r/124643/#comment57739> In one file you have insertMessage, in another logMessage. Some consistency please... plugins/history/chathistoryplugin.cpp (line 23) <https://git.reviewboard.kde.org/r/124643/#comment57728> And if initialization fails? What to do? No, you cannot fail here. Kopete expects that construct do not fail in such case... plugins/history/chathistoryplugin.cpp (line 58) <https://git.reviewboard.kde.org/r/124643/#comment57740> Can you exaplin me what is this line supposed to do? Specially what do you expect in chatMemebers at zero position. plugins/history/chathistoryplugin.cpp (line 66) <https://git.reviewboard.kde.org/r/124643/#comment57729> Ehm.. and what is this? Looks like some misused or wrongly used Kopete::Plugin API. Remove it. plugins/history/databaseconstants.h (line 1) <https://git.reviewboard.kde.org/r/124643/#comment57730> Just remove this whole file and rewrite code to better. Function <name> which just return <name> is useless. Column names are their meanings are already approved/reviewd and are not expected to be changed. In future for some unexpected reasons it will be needed, it can be be fixed... plugins/history/databaseconstants.cpp (line 139) <https://git.reviewboard.kde.org/r/124643/#comment57731> *All* I mean really *all* SQL statements mustn't contains any external data, like return value of function calls or parameters from functions. Fix this problem in *all* files! plugins/history/databaseconstants.cpp (line 144) <https://git.reviewboard.kde.org/r/124643/#comment57732> If some username contains char ' we are on the way to the hell. plugins/history/databasemanager.h (line 53) <https://git.reviewboard.kde.org/r/124643/#comment57733> Missing const for param message? Or why insertMessage needs to modify param message? plugins/history/databasemanager.h (line 62) <https://git.reviewboard.kde.org/r/124643/#comment57741> Now from documentation text, parameters and return value I'm trying to understand: 1) What this function is doing (without looking into code) 2) Why you need such function But I'm not able to answer myself. As return value is list of contacts (and not accounts as written in doc text) I do not know why you need such this in plugin. There is already Kopete API function which returns all contacts for specified account. plugins/history/databasemanager.h (line 69) <https://git.reviewboard.kde.org/r/124643/#comment57742> Same for this function... again documentation is useless and motivation for it too. Parameter contact/contactId is myself() contact? Or remote contact? If remote for which account? plugins/history/databasemanager.h (line 80) <https://git.reviewboard.kde.org/r/124643/#comment57743> Now question for you: After one instance object is created, when is destroyed? You must know it, because there is connection to database which is system resource... plugins/history/databasemanager.cpp (line 25) <https://git.reviewboard.kde.org/r/124643/#comment57744> Hm... What is this function doing? From first look I think that BIG nothing. plugins/history/databasemanager.cpp (line 38) <https://git.reviewboard.kde.org/r/124643/#comment57745> When can it be opened? plugins/history/databasemanager.cpp (line 47) <https://git.reviewboard.kde.org/r/124643/#comment57746> history2 plugin is using exactly same file for database. Please do not use this filename, we can have lot of troubles. Choose different filename. plugins/history/databasemanager.cpp (line 113) <https://git.reviewboard.kde.org/r/124643/#comment57734> Is not isGroup boolean/numeric value? Why passing string? plugins/history/databasemanager.cpp (line 188) <https://git.reviewboard.kde.org/r/124643/#comment57748> And this will break chat history for contacts which name contains char ',' Yes, I have Skype contacts with ',' in their skype user id. plugins/history/databasemanager.cpp (line 193) <https://git.reviewboard.kde.org/r/124643/#comment57747> direction is not string value plugins/history/historybrowser.cpp (line 51) <https://git.reviewboard.kde.org/r/124643/#comment57735> What is this QTextStream doing? Just number to string conversion? For that use QString::number(). plugins/history/historybrowser.cpp (line 52) <https://git.reviewboard.kde.org/r/124643/#comment57736> Are you really sure that these Kopete:: calls always returns valid CSS values encoded for HTML inclusion? I do not think so... plugins/history/historybrowser.cpp (line 95) <https://git.reviewboard.kde.org/r/124643/#comment57749> No! Please do not use these hacks. - Pali Rohár On aug. 6, 2015, 2:10 popoludní, Joseph Joshua wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124643/ > ----------------------------------------------------------- > > (Updated aug. 6, 2015, 2:10 popoludní) > > > Review request for Kopete and Pali Rohár. > > > Repository: kopete > > > Description > ------- > > GSoC 2015: History Plugin > > > Diffs > ----- > > plugins/CMakeLists.txt 86fe572 > plugins/history/CMakeLists.txt 015df95 > plugins/history/chathistoryplugin.h PRE-CREATION > plugins/history/chathistoryplugin.cpp PRE-CREATION > plugins/history/converter.cpp dd56014 > plugins/history/databaseconstants.h PRE-CREATION > plugins/history/databaseconstants.cpp PRE-CREATION > plugins/history/databasemanager.h PRE-CREATION > plugins/history/databasemanager.cpp PRE-CREATION > plugins/history/historybrowser.h PRE-CREATION > plugins/history/historybrowser.cpp PRE-CREATION > plugins/history/historybrowser.ui PRE-CREATION > plugins/history/historychatui.rc 6e2c072 > plugins/history/historyconfig.kcfg 0606910 > plugins/history/historydialog.h 5ba20ce > plugins/history/historydialog.cpp 0720cb7 > plugins/history/historyguiclient.h 35a32ab > plugins/history/historyguiclient.cpp 08b5816 > plugins/history/historyimport.h a922df7 > plugins/history/historyimport.cpp 9d9aa15 > plugins/history/historylogger.h e032f2f > plugins/history/historylogger.cpp 8a34c85 > plugins/history/historyplugin.h 35cc4f9 > plugins/history/historyplugin.cpp bedd560 > plugins/history/historypreferences.h e74a3b7 > plugins/history/historypreferences.cpp c10005c > plugins/history/historypreferencesui.ui PRE-CREATION > plugins/history/historyprefsui.ui 6cd3b87 > plugins/history/historyui.rc 5f72b22 > plugins/history/historyviewer.ui e730b20 > plugins/history/kopete_history.desktop b96742a > plugins/history/kopete_history_config.desktop 337e943 > plugins/history2/CMakeLists.txt 195edf6 > plugins/history2/history2chatui.rc 6e2c072 > plugins/history2/history2config.kcfg 17055b6 > plugins/history2/history2config.kcfgc e87f8ca > plugins/history2/history2dialog.h 29b1b5c > plugins/history2/history2dialog.cpp e5fa8a2 > plugins/history2/history2guiclient.h d07ce18 > plugins/history2/history2guiclient.cpp 6a4c4c5 > plugins/history2/history2import.h c2760f6 > plugins/history2/history2import.cpp db103fe > plugins/history2/history2logger.h 13f159a > plugins/history2/history2logger.cpp cad59df > plugins/history2/history2plugin.h 424861a > plugins/history2/history2plugin.cpp ca4d580 > plugins/history2/history2preferences.h 2782254 > plugins/history2/history2preferences.cpp 59dfb8c > plugins/history2/history2prefsui.ui b2ffab2 > plugins/history2/history2ui.rc 5f72b22 > plugins/history2/history2viewer.ui eb670fe > plugins/history2/kopete_history2.desktop 7bee952 > plugins/history2/kopete_history2_config.desktop 7c39892 > > Diff: https://git.reviewboard.kde.org/r/124643/diff/ > > > Testing > ------- > > -Builds successfully > -Chats are logged into the database > -Chat history is appended to new chats > -Chat history can be browsed > > > Thanks, > > Joseph Joshua > >
_______________________________________________ kopete-devel mailing list kopete-devel@kde.org https://mail.kde.org/mailman/listinfo/kopete-devel
