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/#review83542
-----------------------------------------------------------



plugins/history/chathistoryplugin.cpp (line 23)
<https://git.reviewboard.kde.org/r/124643/#comment57785>

    You generally shouldn't perform external calls in object constructor. The motivation is inability to somehow report call failure to the constructor caller. Consider either moving database initialization into a separate function (which is to be called after an object is constructed) or perform an on-demand initialization.
    
    I'd also avoid singleton DatabaseManager. Consider using dependency injection instead as singletons mess with unit testing.



plugins/history/chathistoryplugin.cpp (line 43)
<https://git.reviewboard.kde.org/r/124643/#comment57786>

    It's 'show', not 'view'. By calling a 'view' method you're asking HistoryPlugin object to look at your history, and 'show' call asks HistoryPlugin to show history to the caller.



plugins/history/chathistoryplugin.cpp (line 45)
<https://git.reviewboard.kde.org/r/124643/#comment57787>

    You create an object, store it into the local variable and then abandon this variable. Some memory cleanup should occur, as far as I recall the C++.



plugins/history/chathistoryplugin.cpp (line 49)
<https://git.reviewboard.kde.org/r/124643/#comment57788>

    It's called wrong. This function actually performs a View population with history info, consider a more specific naming.


- 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