Review Request 116915: Fix Bug 305618

[Thorsten Zachmann]

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


You are on the right track. I have pointed out some small stuff to fix and then it can be committed. Good work.


stage/part/tools/animationtool/KPrPageEffectDocker.h
<https://git.reviewboard.kde.org/r/116915/#comment37710>

    The function name should start with a lowercase letter.



stage/part/tools/animationtool/KPrPageEffectDocker.cpp
<https://git.reviewboard.kde.org/r/116915/#comment37713>

    The command text "Apply To All" is to generic and does not realy say what it is about. It would be better changed to "Apply Slide Effect to all Pages"
    



stage/part/tools/animationtool/KPrPageEffectDocker.cpp
<https://git.reviewboard.kde.org/r/116915/#comment37711>

    Getting the factory is better done outside the foreach loop as it is not dependend on the page. This will be faster as the code is only executed once.



stage/part/tools/animationtool/KPrPageEffectDocker.cpp
<https://git.reviewboard.kde.org/r/116915/#comment37712>

    A small stype issue. There should be no space before (. There should be also no space after ( and before ). I know the code you are in has quite a lot of those but it dates back to a time were we did not yet use the code style.


- Thorsten Zachmann


On March 23, 2014, 2:47 a.m., Wenchao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116915/
> -----------------------------------------------------------
> 
> (Updated March 23, 2014, 2:47 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> The patch tries to add a new function about "add this slide animation for all slides". After the checkbox is checked, the presentation will use the same slide transition for all slides.
> The function "slotApplyToAllSlides()" is defined in file KPrPageEffectDocker.cpp which implements the task.
> 
> 
> Diffs
> -----
> 
>   stage/part/tools/animationtool/KPrPageEffectDocker.cpp 836d687 
>   stage/part/commands/KPrPageEffectSetCommand.h ec95f07 
>   stage/part/commands/KPrPageEffectSetCommand.cpp f44953f 
>   stage/part/tools/animationtool/KPrPageEffectDocker.h e7a5277 
> 
> Diff: https://git.reviewboard.kde.org/r/116915/diff/
> 
> 
> Testing
> -------
> 
> Tested manually.
> 
> 
> Thanks,
> 
> Wenchao Li
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20140323/218e7546/attachment.htm>