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
