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

Mon Jul 11 05:00:48 BST 2011

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

Some things I noticed when testing:

The custom slide show name is not fully readable when the name is to long for the combobox. The combo box should use a bigger width if available (not the full width that is available) so that the full name is readable. At the moment it is just to small.

It is not possible to add the same slide to a custom slide show multiple time. That should be possible as the user does not need to create copies of the slide if he wants to use it multiple times.

The animation that is show when a custom slide show gets selected should be faster. At the moment it takes quite some time.

The old way of adding custom slide shows should be removed the same time the new custom slide shows are added. To make the feature easier to find the menu entry in Edit menu should bring you to the new stuff.

If a slide is deleted (e.g. in the normal slide sorter) that is part of a custom slide show it is not readadded to the custom slide show when the delete of the slide is undone.

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

    the variable name c_action sounds a bit stange and it does not say much. How about renaming it to action

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

    This is not needed please remove

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

    No need to create a copy a format. Just use types[0] at the place where format is used.

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

    "New Slide Show X" does not sound very good as a slide show name. How about using "Slide Show %1"

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

    Here you should use || instead of |

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

    The text should be change to "Add custom slide show". The new is not needed.

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

    I'm not sure if we should use Remove here. Can you please check if we otherwise always use Delete. If so it should use Delete here to.

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

    There should be no blank before )

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

    I guess that should be "Edit custom slide show" as it only is about one slide show and not multiple

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

    Please remove the empty line

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

    It would be goof to check all slides here. So you see the position of all slides is correct.

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

    you can check the full list here
    QCOMPARE(modifiedSlideShow, initialSlideShow);

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

    Better use 2 tests here. Then it is easier when one fails to see which one fails. Also using a QCOMPARE might be better.

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

    Better use 2 tests here. Then it is easier when one fails to see which one fails. Also using a QCOMPARE might be better.

- Thorsten

On July 8, 2011, 8:02 p.m., Paul Mendez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101820/
> -----------------------------------------------------------
> 
> (Updated July 8, 2011, 8:02 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/TestAddCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/tests/TestAddCustomSlideShowCommand.cpp PRE-CREATION 
>   kpresenter/part/tests/TestDelCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/tests/TestDelCustomSlideShowCommand.cpp PRE-CREATION 
>   kpresenter/part/tests/TestEditCustomSlideShowsCommand.h PRE-CREATION 
>   kpresenter/part/tests/TestEditCustomSlideShowsCommand.cpp PRE-CREATION 
>   kpresenter/part/tests/TestRenameCustomSlideShowCommand.h PRE-CREATION 
>   kpresenter/part/tests/TestRenameCustomSlideShowCommand.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/20110711/a63fa56e/attachment.htm>