Review Request: Stage: GSoC: New GUI for manage custom slide shows

Sat Jul 2 05:30:02 BST 2011

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

I have not tested yet but will do so as soon as I find the time for it. Looks like you where really busy and I'm looking forward to test this

kpresenter/part/KPrCustomSlideShows.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3533>

    There should be no blank after the *

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3534>

    There is no need to number the enum in this case as far as I can see.

    Please use CamelCase for the values. So it should be SlidesAdd instead of SLIDES_ADD

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3535>

    If it will not be used later plase remove.

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3538>

    There should be no blank before (

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3537>

    This function should be made const

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3536>

    The name should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3539>

    The size should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3540>

    The parameters should be passed as const reference.

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3541>

    The parameter should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3542>

    The parameter should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3543>

    The parameter pages should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3544>

    The parameter slides should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3545>

    The parameter name should be passed as const reference

kpresenter/part/KPrCustomSlideShowsModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3546>

    Please remove the additional blanks at the end of the line

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3547>

    There should be no blank before the (

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3548>

    The variable can be made const as it is not modified.

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3549>

    that should be || instead of |

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3550>

    It would be good to initialize the value to a default as otherwise it is not defined. It is not a problem in this case as it will be made sure it is set later before it is used.

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3553>

    What is the resaon for this ordering? 

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3552>

    Please use brackes also for one line statements

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3551>

    Can both formats happen at the same time. If not please use a else if as it is faster in the case the first one was already used.

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3555>

    this part is nearly the same and I think it can be refactored to be there only once by moving it out of the if and setting having the action in a variable.

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3556>

    This should be || instead of |

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3557>

    As fas as I can see the reset only needs to be does in the slideshow exists so it should be move up before the bracket

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3558>

    The if is not needed here. It does not matter when it gets set with the same value.

kpresenter/part/KPrCustomSlideShowsModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3559>

    Please use also brackets here.

kpresenter/part/KPrSlidesManagerView.h
<http://git.reviewboard.kde.org/r/101820/#comment3560>

    This line might no longer be needed when the toolProxy is removed

kpresenter/part/KPrSlidesManagerView.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3561>

    This include might no longer be needed.

kpresenter/part/KPrSlidesManagerView.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3562>

    It might be a good idea to have constants for this fixed number 10 with a meaningful name. That makes it much easier for people to understand the values. Also it makes it much easier to change them if not all places need to be update by hand.

kpresenter/part/KPrSlidesManagerView.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3563>

    for the 23 it goes the same as for the 10 above.

kpresenter/part/KPrSlidesSorterDocumentModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3564>

    The parameter should be passed as const reference

kpresenter/part/KPrSlidesSorterDocumentModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3565>

    The parameter should be passed as const reference 

kpresenter/part/KPrSlidesSorterDocumentModel.h
<http://git.reviewboard.kde.org/r/101820/#comment3566>

    The parameter should be passed as const reference 

kpresenter/part/KPrSlidesSorterDocumentModel.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3567>

    The indention is not correct in this line

kpresenter/part/KPrViewModeSlidesSorter.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3568>

    If you do a static cast you don't need to check. In this case please remove the check as the static_cast is correct.

kpresenter/part/KPrViewModeSlidesSorter.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3569>

    I guess the & should be a &&.

kpresenter/part/commands/KPrAddCustomSlideShowCommand.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3571>

    The parameter name should be passed as const reference

kpresenter/part/commands/KPrAddCustomSlideShowCommand.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3570>

    This empty line can be removed

kpresenter/part/commands/KPrDelCustomSlideShowCommand.h
<http://git.reviewboard.kde.org/r/101820/#comment3572>

    The parameter name should be passed as const reference

kpresenter/part/commands/KPrDelCustomSlideShowCommand.h
<http://git.reviewboard.kde.org/r/101820/#comment3573>

    The variable is only set but never read so it is not needed and can be removed

kpresenter/part/commands/KPrEditCustomSlideShowsCommand.h
<http://git.reviewboard.kde.org/r/101820/#comment3574>

    The parameter name should be passed as const reference

kpresenter/part/tests/TestEditCustomSlideShowsCommand.cpp
<http://git.reviewboard.kde.org/r/101820/#comment3575>

    It is better to use QCOMPARE when comparing 2 values as thne it also print out the difference in case of a failue.

- Thorsten

On July 1, 2011, 10:11 p.m., Paul Mendez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101820/
> -----------------------------------------------------------
> 
> (Updated July 1, 2011, 10:11 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> (preliminary) new GUI for manage custom slide shows. 
> Note: As some GUI integration is still missing, it's only a review request, it isn't for merge with master branch.
> (part of GSoC project)
> 
> 
> Diffs
> -----
> 
>   kpresenter/part/CMakeLists.txt 613ff8d3de84a19776df1651c0bd3604fc395099 
>   kpresenter/part/KPrCustomSlideShows.h 7d8a91b67e14e32312fd2706dcede6efac6e1aa7 
>   kpresenter/part/KPrCustomSlideShows.cpp 4088a7446d2992d3d56059bec4ad646b4bbfc77f 
>   kpresenter/part/KPrCustomSlideShowsModel.h PRE-CREATION 
>   kpresenter/part/KPrCustomSlideShowsModel.cpp PRE-CREATION 
>   kpresenter/part/KPrSlidesManagerView.h 6f345e049cdf746e00fdfd9e3e7644668ff9e96e 
>   kpresenter/part/KPrSlidesManagerView.cpp 39e80b1b770584f97e6bfcbcebc06bd107bc1a68 
>   kpresenter/part/KPrSlidesSorterDocumentModel.h a5b65a4d87f8af6a51b4e1992c2ac2a3d23aed37 
>   kpresenter/part/KPrSlidesSorterDocumentModel.cpp 4938e3696bed3fb90c017aba5997c40268ce3e99 
>   kpresenter/part/KPrSlidesSorterItemDelegate.h PRE-CREATION 
>   kpresenter/part/KPrSlidesSorterItemDelegate.cpp PRE-CREATION 
>   kpresenter/part/KPrViewModeSlidesSorter.h 240bd9e0c6b4158ff0726f2e72e4c79982a9ea18 
>   kpresenter/part/KPrViewModeSlidesSorter.cpp 61866f8a1344e1999224444d71a46a1a21a1864e 
>   kpresenter/part/commands/KPrAddCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/commands/KPrAddCustomSlideShowCommand.cpp PRE-CREATION 
>   kpresenter/part/commands/KPrDelCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/commands/KPrDelCustomSlideShowCommand.cpp PRE-CREATION 
>   kpresenter/part/commands/KPrEditCustomSlideShowsCommand.h PRE-CREATION 
>   kpresenter/part/commands/KPrEditCustomSlideShowsCommand.cpp PRE-CREATION 
>   kpresenter/part/commands/KPrRenameCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/commands/KPrRenameCustomSlideShowCommand.cpp PRE-CREATION 
>   kpresenter/part/tests/CMakeLists.txt f9b56a6ed55ebe610ae30cd21437975c00c317d1 
>   kpresenter/part/tests/PAMock.h PRE-CREATION 
>   kpresenter/part/tests/TestEditCustomSlideShowsCommand.h PRE-CREATION 
>   kpresenter/part/tests/TestEditCustomSlideShowsCommand.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101820/diff
> 
> 
> Testing
> -------
> 
> build and run
> 
> 
> Thanks,
> 
> Paul
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110702/42aea953/attachment.htm>