----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124643/#review83547 -----------------------------------------------------------
plugins/history/databaseconstants.cpp (line 117) <https://git.reviewboard.kde.org/r/124643/#comment57792> Postgres, for example, does not support AUTOINCREMENT, providing powerful sequencing capabilities instead. Make sure you won't rewrite such SQL statements every time a new DBMS support is added to Kopete History. plugins/history/databasemanager.h (lines 32 - 35) <https://git.reviewboard.kde.org/r/124643/#comment57793> That's the hell of a comment. Consider explaining what exactly does this destructor do instead of announcing it's a destructor. plugins/history/databasemanager.h (lines 37 - 45) <https://git.reviewboard.kde.org/r/124643/#comment57794> You should also explain what happens in case of a sequence of successive initDatabase calls (possibly with different DatabaseType). It's a bad class, because it's a singleton (one per-application) allowing us to work with one and only one of several DBMS available (DatabaseType presence tells us there will be more databases to support). What if I'll need to use two different DB engines? Consider writing a reusable code. plugins/history/databasemanager.h (line 95) <https://git.reviewboard.kde.org/r/124643/#comment57795> Error messages are usually handing around with error codes which are really easier to analyze programmatically. Consider asking Pali if KDE already provides such error coding capability and, if so, add error code here. plugins/history/databasemanager.cpp (lines 52 - 57) <https://git.reviewboard.kde.org/r/124643/#comment57796> Will you duplicate this check for every DatabaseType available? plugins/history/databasemanager.cpp (line 90) <https://git.reviewboard.kde.org/r/124643/#comment57799> You should never compare enumerations like that as it disables compiler checks on enums handling completeness. Consider using switch extracted into a separate function. plugins/history/databasemanager.cpp (line 91) <https://git.reviewboard.kde.org/r/124643/#comment57798> Same question Pali asked about zero contact not explained. plugins/history/databasemanager.cpp (line 154) <https://git.reviewboard.kde.org/r/124643/#comment57797> And what if db is closed? - Roman Nazarenko On Авг. 6, 2015, 12:10 п.п., Joseph Joshua wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/124643/ > ----------------------------------------------------------- > > (Updated Авг. 6, 2015, 12:10 п.п.) > > > 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
