[Marble-devel] Review Request 114915: [ESA SoCIS 2013] Eclipses event reminder dialog with application logic

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/114915/#review47088
-----------------------------------------------------------


Merely a quick code style review.

Can you please add curly brackets in the sense of "Use curly braces even when the body of a conditional statement contains only one line." everyhwere. E.g. 

if ( foo ) 
  bar = 0;

becomes

if ( foo ) {
  bar = 0;
}



src/plugins/render/eclipses/EclipsesFilterProxyModel.h
<https://git.reviewboard.kde.org/r/114915/#comment33553>

    Please move all inline implementations to the .cpp file.
    



src/plugins/render/eclipses/EclipsesFilterProxyModel.h
<https://git.reviewboard.kde.org/r/114915/#comment33554>

    Strange naming. m_eclipsesModel?



src/plugins/render/eclipses/EclipsesItem.h
<https://git.reviewboard.kde.org/r/114915/#comment33555>

    Seems to violate the rule of three, is this intended?
    http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)



src/plugins/render/eclipses/EclipsesModel.h
<https://git.reviewboard.kde.org/r/114915/#comment33556>

    Same here (rule of three): what about the copy assignment operator?
    



src/plugins/render/eclipses/EclipsesModel.cpp
<https://git.reviewboard.kde.org/r/114915/#comment33557>

    This will crash given the current implementation of the destructor (m_ecl pointer is copied here instead of creating a new object, destructor deletes m_ecl. Two or more objects then will lead to double deletion).
    



src/plugins/render/eclipses/EclipsesPlugin.cpp
<https://git.reviewboard.kde.org/r/114915/#comment33558>

    NULL => 0
    http://www.stroustrup.com/bs_faq2.html#null



src/plugins/render/eclipses/EclipsesPlugin.cpp
<https://git.reviewboard.kde.org/r/114915/#comment33559>

    Aren't such checks the duty of the respective setter (if needed at all)?



src/plugins/render/eclipses/EclipsesReminderDialog.cpp
<https://git.reviewboard.kde.org/r/114915/#comment33560>

    Please don't abuse int for bool



src/plugins/render/eclipses/EclipsesReminderDialog.cpp
<https://git.reviewboard.kde.org/r/114915/#comment33561>

    No abbreviations please. s => model


- Dennis Nienhüser


On Jan. 8, 2014, 9:44 p.m., Marek Hakala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114915/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2014, 9:44 p.m.)
> 
> 
> Review request for Marble, Torsten Rahn and René Küttner.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> 
> Task:
> Implement the reminder functionality. There already is a reminder option in the preferences dialog and a user interface for the reminder dialog (EclipsesReminderDialog.ui). However, these are not implemented yet. You could implement the code that reminds the user if an eclipse event occours within the next week and show the dialog. It would also be nice if the user could jump to the eclipse event from the reminder dialog. We also have to make sure the dialog does not appear again and again. The dialog itself should show some information about the event (type, date, time, length etc.)
> 
> Solution:
> In this task I implemented the filter model and application logic for reminder by the QT's Model/View patterns. I added the facade class for reminder management. This patch adds the ability for remind eclises events in adjustable time period.
> 
> 
> Diffs
> -----
> 
>   src/plugins/render/eclipses/CMakeLists.txt f650e08 
>   src/plugins/render/eclipses/EclipsesConfigDialog.ui ca25e6f 
>   src/plugins/render/eclipses/EclipsesFilterProxyModel.h PRE-CREATION 
>   src/plugins/render/eclipses/EclipsesFilterProxyModel.cpp PRE-CREATION 
>   src/plugins/render/eclipses/EclipsesItem.h af59d5a 
>   src/plugins/render/eclipses/EclipsesItem.cpp 4c107b9 
>   src/plugins/render/eclipses/EclipsesModel.h 1728aa1 
>   src/plugins/render/eclipses/EclipsesModel.cpp 93f19dc 
>   src/plugins/render/eclipses/EclipsesPlugin.h 5484292 
>   src/plugins/render/eclipses/EclipsesPlugin.cpp 65c1326 
>   src/plugins/render/eclipses/EclipsesReminderDialog.h PRE-CREATION 
>   src/plugins/render/eclipses/EclipsesReminderDialog.cpp PRE-CREATION 
>   src/plugins/render/eclipses/EclipsesReminderDialog.ui 5d17519 
>   src/plugins/render/eclipses/EclipsesReminderFacade.h PRE-CREATION 
>   src/plugins/render/eclipses/EclipsesReminderFacade.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/114915/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Marek Hakala
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20140108/03cb0185/attachment.html>