Re: Review Request 124643: First review request for the history plugin

-----------------------------------------------------------
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
[email protected]
https://mail.kde.org/mailman/listinfo/kopete-devel