Review Request: GSoC: improve selection, drag&drop of Stage Slides Sorter

Tue Jun 7 04:37:57 BST 2011

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

Ship it!

Nice work Paul. Sorry that it tool so long. I have added some comments. The most of those are style issues. There are also some comments how the code can be simplified. I will also give it a try today or tomorrow, but I had compiled your branch some days ago and it was working nicely. After you have addressed the things I pointed out please feel free to commit or update the patch if you prefer.

kpresenter/part/KPrSelectionManager.h
<http://git.reviewboard.kde.org/r/101501/#comment3047>

    Please make the QAbstractItemView the last parameter to be more in line how it is handled e.g. in qt. Also the document is needed so please remove the default parameter

kpresenter/part/KPrSelectionManager.h
<http://git.reviewboard.kde.org/r/101501/#comment3048>

    The second private is not needed. Please remove.

kpresenter/part/KPrSelectionManager.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3049>

    Is this header needed?

kpresenter/part/KPrSelectionManager.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3050>

    Most but not all of these headers are not needed. Please remove unneeded header as it speeds up compilation.

kpresenter/part/KPrSelectionManager.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3051>

    Please move the : to the next line

kpresenter/part/KPrSelectionToggle.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3052>

    Seems not to be used please remove.

kpresenter/part/KPrSelectionToggle.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3053>

    Seems not to be used please remove.

kpresenter/part/KPrSelectionToggle.cpp
<http://git.reviewboard.kde.org/r/101501/#comment3054>

    Please move the : to the next line

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

    Please remove the blanks before and after (

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

    Please remove the blanks before/after ( and )

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

    The method should be const

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

    These two seem not to be used please remove.

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

    Please don't include QtGui but instead include the classes needed.

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

    Is this include needed?

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

    Please use true instead of TRUE

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

    Please remove this empty line as it does not help structuring the code

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

    Please moe the * to the variable so that it is QDrag *drag.

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

    Please add { }  also to one line if statements

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

    The indention is one to short.

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

    I think you can replace that by 
    int yCount = qCeil(itemCount/xCount));

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

    Please add blanks around /

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

    Please add blanks around /

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

    Please add blanks around /

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

    This can be simplified to return QPair<int,int>(numberColumn, numberRow);

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

    These two are not used please remove.

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

    Please add { } also for one line if statements.

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

    Please put the opening { in the same line as the if

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

    Please add { } also for one line if statements.

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

    Please remove the blank after toInt

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

    Please remove the blnak before ()

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

    Please remove the empty line. One empty line is enough.

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

    please remove the blank after ( and before )

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

    There should be no blank before the : so it should be 

    case Qt::MoveAction: inteard of  case Qt::MoveAction :

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

    Please remove the commented out line

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

    Please rmove the blanks after ( and before ). Also please make sure to always  use only one blank

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

    This can be simplified to 

    QKeyEvent *keyEv = dynamic_cast<QKeyEvent *>(event);

    if (keyEv && keyEv->key() == Qt::Key_Delete) {
        deleteSlide()
    }

    However if you think you will need to support more keys or other events keep it as it is.

- Thorsten

On June 4, 2011, 3:35 a.m., Paul Mendez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101501/
> -----------------------------------------------------------
> 
> (Updated June 4, 2011, 3:35 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> Add +/- buttons for selection in a dolphin style
> Add context menu when dropping and key action for move/copy
> Add thumbnails of slides when dragging
> Refactor View of slides list for reuse.
> Refactor Document Model to manage drag&drop instead of the view.
> 
> 
> Diffs
> -----
> 
>   kpresenter/part/CMakeLists.txt a2b01eee13eee40949f177dd85e13d3683bc984f 
>   kpresenter/part/KPrSelectionManager.h PRE-CREATION 
>   kpresenter/part/KPrSelectionManager.cpp PRE-CREATION 
>   kpresenter/part/KPrSelectionToggle.h PRE-CREATION 
>   kpresenter/part/KPrSelectionToggle.cpp PRE-CREATION 
>   kpresenter/part/KPrSlidesManagerView.h PRE-CREATION 
>   kpresenter/part/KPrSlidesManagerView.cpp PRE-CREATION 
>   kpresenter/part/KPrSlidesSorterDocumentModel.h 71bf8cc309249ed50ab125a7c9e0f32f4de225a3 
>   kpresenter/part/KPrSlidesSorterDocumentModel.cpp d9ddf0fbf5833bb05df5cfbb022038e3ccb99c27 
>   kpresenter/part/KPrViewModeSlidesSorter.h 917bd374a8445f37eac74e862f2c7b75a97f321f 
>   kpresenter/part/KPrViewModeSlidesSorter.cpp b71c1b995683d134f419e31ae37d20148139e15f 
>   libs/kopageapp/KoPAView.cpp b60c6a47475b8ab07153327daeb5286df9d061a1 
> 
> Diff: http://git.reviewboard.kde.org/r/101501/diff
> 
> 
> Testing
> -------
> 
> build and run
> 
> 
> Thanks,
> 
> Paul
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20110607/23eadd1c/attachment.htm>