D8948: Created an auxiliary function 'KFilePlacesModel::movePlace'

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:1039
> +
> +    //use a invalid start postion
> +    QVERIFY(!m_places->movePlace(100, 20));

typo: position

> kfileplacesmodeltest.cpp:1041
> +    QVERIFY(!m_places->movePlace(100, 20));
> +    QTRY_COMPARE(rowsMoved.count(), 0);
> +

It was 0 already, so the TRY_ is unnecessary, if it's 0 it's ok right away, and if it's 1 then waiting more won't change it back to 0 ;)

> kfileplacesmodeltest.cpp:1045
> +    QVERIFY(!m_places->movePlace(1, 1));
> +    QTRY_COMPARE(rowsMoved.count(), 0);
> +}

remove TRY_

> kfileplacesmodel.cpp:635
> +    int direction = (target - source);
> +    if (direction > 0) { // moving down, move it to the end of the group
> +        int newTarget = source;

why not just `if (target > source)` ? You're not using `direction` anywhere else.

> kfileplacesmodel.cpp:636
> +    if (direction > 0) { // moving down, move it to the end of the group
> +        int newTarget = source;
> +        while(items.at(newTarget)->groupType() == groupType) {

Urgh, this shadows `newTarget` from the outer scope!
That's hard to read and feels like a bug.
Rename one of the variables.

> kfileplacesmodel.cpp:637
> +        int newTarget = source;
> +        while(items.at(newTarget)->groupType() == groupType) {
> +            newTarget++;

missing space after while  (repeats)

> kfileplacesmodel.cpp:649
> +            newTarget--;
> +            // begining of the list move it there
> +            if (newTarget == 0) {

typo: beginning

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D8948

To: renatoo, dfaure
Cc: #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171122/6a6a2787/attachment-0001.html>