----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/124643/#review83555 -----------------------------------------------------------
plugins/history/historybrowser.h (line 40) <https://git.reviewboard.kde.org/r/124643/#comment57821> I always recall scholar Delphi times when I see such a function. onButton1Clicked. Function should tell a reader what it's doing. plugins/history/historybrowser.cpp (lines 32 - 34) <https://git.reviewboard.kde.org/r/124643/#comment57822> In C++ you should declare variables as needed, not in a header of a function. This way uninitialized variables cannot be misused. plugins/history/historybrowser.cpp (lines 44 - 45) <https://git.reviewboard.kde.org/r/124643/#comment57823> Those are called 'magic numbers'. There's really no way to find out why should view margins be 4. Extract them with proper naming and documentation. plugins/history/historybrowser.cpp (lines 52 - 55) <https://git.reviewboard.kde.org/r/124643/#comment57825> Consider excluding all the low-level HTML logic into a separate function. plugins/history/historybrowser.cpp (line 55) <https://git.reviewboard.kde.org/r/124643/#comment57824> The PHP experience. Isn't there any templating capability in KHTMLPart? plugins/history/historybrowser.cpp (line 59) <https://git.reviewboard.kde.org/r/124643/#comment57826> Do we really 'display' them, or just setup? plugins/history/historybrowser.cpp (line 72) <https://git.reviewboard.kde.org/r/124643/#comment57827> What is trv? How will I find out an answer with you being unavailable? plugins/history/historybrowser.cpp (lines 73 - 76) <https://git.reviewboard.kde.org/r/124643/#comment57828> Magic numbers again. plugins/history/historybrowser.cpp (lines 87 - 89) <https://git.reviewboard.kde.org/r/124643/#comment57829> Magic, magic is everywhere. plugins/history/historybrowser.cpp (line 93) <https://git.reviewboard.kde.org/r/124643/#comment57834> This function is huge and thus it's not readable at all. Consider refactoring it. plugins/history/historybrowser.cpp (lines 97 - 98) <https://git.reviewboard.kde.org/r/124643/#comment57830> Too much spaces. plugins/history/historybrowser.cpp (line 98) <https://git.reviewboard.kde.org/r/124643/#comment57831> Magic. I'd see someone who needs to change all 99's to 98's. plugins/history/historybrowser.cpp (line 137) <https://git.reviewboard.kde.org/r/124643/#comment57832> enum comparison again. plugins/history/historybrowser.cpp (lines 153 - 155) <https://git.reviewboard.kde.org/r/124643/#comment57833> Magic-powered templating. You should never mix HTML with C++ code. plugins/history/historyconfig.kcfg (line 2) <https://git.reviewboard.kde.org/r/124643/#comment57820> Have you just removed authoring notice from GPL licensed software? This is bad. - 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
