Review Request: Plasmate:migrate timeline.cpp from Q classes to K classes

[Aaron J. Seigo]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102027/#review4957
-----------------------------------------------------------



savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/102027/#comment4345>

    unless you are using the features from KAction or KMenu, there is no reason to make this change. KMenu provides titles and KAction is used for advanced shortcuts and system authentication integration.
    
    this change doesn't hurt anything, but just so you don't invest too much of your time in making changes that don't result in actual benefits. :)



savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/102027/#comment4348>

    was there a particular reason why this message box was removed?



savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/102027/#comment4347>

    Yes/No buttons are not great usuability; it is usually preferred to have them describe the action.
    
    e.g. in this case the Yes button could be set to say "Remove the selected section." and no should probably be "Cancel"



savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/102027/#comment4346>

    while not your fault (the code was already there), there's a common mistake here: using dynamic_cast, which can return NULL, and then not checking the value before use. this should either be written as:
    
    KAction *sender = static_cast<KAction *>(this->sender());
    const QString branch = sender->text();
    
    or:
    
    
    KAction *sender = dynamic_cast<KAction *>(this->sender());
    if (sender) {
        const QString branch = sender->text();
         ....
    }
    
    also, in this case, using a qobject_cast rather than dynamic_cast is also usually prefered.


- Aaron J.


On July 21, 2011, 3:44 p.m., Giorgos Tsiapaliwkas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102027/
> -----------------------------------------------------------
> 
> (Updated July 21, 2011, 3:44 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> hello,
> 
> the patch migrates the timeline.cpp from the Q classes to K classes.
> Also adds a KMessageBox::information in the newsavepoint(),in order to inform the user that he can't create a new save point without any changes being made
> 
> 
> Diffs
> -----
> 
>   savesystem/timeline.cpp 1d7bf03 
> 
> Diff: http://git.reviewboard.kde.org/r/102027/diff
> 
> 
> Testing
> -------
> 
> no issues.
> 
> 
> Thanks,
> 
> Giorgos
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110722/a18f98d2/attachment-0001.htm