Merge lp:~uriboni/unity-2d/support-removeRows-in-models into lp:unity-2d/3.0

[Ugo Riboni]

Add support for removing rows by implementing the virtual method QAbstractItemModel::removeRows in WindowsList and ListAggregatorModel.
Make the internal list in WindowsList protected instead of private.

Add support for removing rows by implementing the virtual method QAbstractItemModel::removeRows in WindowsList and ListAggregatorModel.

Make the internal list in WindowsList protected instead of private so that it is easier for other classes to inherit from this model to do useful things.

This change is not currently needed by Unity-2d, but I do need it for another project.
It can be however pretty useful for Unity-2d too in the future as it's just standard functionality on the models that wasn't simply implemented before.

[Florian Boucault (fboucault)]

In WindowsList::removeRows:
- a debug statement was leftover
- using removeAt would probably simplify the code quite a lot

review: Needs Fixing

[lp:~uriboni/unity-2d/support-removeRows-in-models]

584. By Ugo Riboni on 2011-06-06

Remove debug statement

585. By Ugo Riboni on 2011-06-06

Merge changes from trunk

586. By Ugo Riboni on 2011-06-06

Fix a bug in the ListAggregatorModel removal code. Simplify the WindowsList removal code.

587. By Ugo Riboni on 2011-06-07

Fix missing brace

[Olivier Tilloy (osomon)]

Some style nitpicking to get started with.

+bool ListAggregatorModel::removeRows(int row, int count,
+ const QModelIndex& parent)

I know this isn’t in the style guidelines, but all the other methods in this file have their return type declared on a separate line, please keep it consistent. If we want to change this at some point, we’ll batch it, but it’s important to keep the whole file consistent for improved readability.
Also, the list of parameters doesn’t look that long to me that it should be cut into two lines.

-
+#include <QList>

Please add back the blank line that separated those groups of #includes.

review: Needs Fixing (code)

[Olivier Tilloy (osomon)]

In ListAggregatorModel::removeRows, I think the following line is wrong:

    int removeCount = qMin(count, model->rowCount() - removeAt);

It should probably be:

    int removeCount = qMin(count - removed, model->rowCount() - removeAt);

review: Needs Fixing

[Olivier Tilloy (osomon)]

In ListAggregatorModel::removeRows, the first test (if (row < 0)) is incomplete, it should also test whether row >= rowCount() or count <= 0, just like in the implementation of WindowsList::removeRows(…).

review: Needs Fixing

[lp:~uriboni/unity-2d/support-removeRows-in-models]

588. By Ugo Riboni on 2011-06-08

Add more proper boundary checks

589. By Ugo Riboni on 2011-06-08

Fix cosmetic code issues

590. By Ugo Riboni on 2011-06-09

Fix an error in the calculation of where to start removing items

[Ugo Riboni (uriboni)]

I fixed all your remarks.
While testing some more I also uncovered a bug in the way the position of the next item to be removed was calculated in cases where the removal spans more than one model.

[Olivier Tilloy (osomon)]

You didn’t take into account my first style nitpick:

« I know this isn’t in the style guidelines, but all the other methods in this file have their return type declared on a separate line, please keep it consistent. If we want to change this at some point, we’ll batch it, but it’s important to keep the whole file consistent for improved readability. »

Other typo remarks:

"By taking that into accountcalculation" : missing whitespace

s/semplified/simplified/

[Olivier Tilloy (osomon)]

In libunity-2d-private/Unity2d/windowslist.h, on the blank line before the 'protected' keyword, you added 4 extra whitespaces, please remove them.

review: Needs Fixing

[Olivier Tilloy (osomon)]

+ if (row < 0 || row >= rowCount(parent) || count <= 0) {

In rowCount(…), the 'parent' argument is unused, and it defaults to QModelIndex(), so you can safely call rowCount() without any argument.

[Olivier Tilloy (osomon)]

+ /* Please note that the offset of the current model is computed
+ by iterating over all previous models and making a sum of their
+ row count.
+ By taking that into accountcalculation for removeAt would be:
+ (row + removed) - (offset + removed)
+ This can be semplified to just row - offset as you see below */
+ int offset = computeOffset(model);
+ int removeAt = row - offset;

This is correct, but I don’t think it’s necessary to be that verbose. I don’t think this comment is needed at all (feel free to keep it if you disagree). What we really need is unit tests…

[lp:~uriboni/unity-2d/support-removeRows-in-models]

591. By Ugo Riboni on 2011-06-09

Fix more code style remarks

[Ugo Riboni (uriboni)]

Fixed the rest of the problems hopefully. I left the verbose comment. It will help me when i look at that code one month down the road.

[Olivier Tilloy (osomon)]

Good to go now!
Did I mention that we need unit tests? ;)

review: Approve

=== modified file 'libunity-2d-private/Unity2d/listaggregatormodel.cpp'
--- libunity-2d-private/Unity2d/listaggregatormodel.cpp	2011-06-07 16:34:46 +0000
+++ libunity-2d-private/Unity2d/listaggregatormodel.cpp	2011-06-09 15:57:26 +0000
@@ -218,3 +218,44 @@
 {
     return data(QAbstractListModel::index(row), 0);
 }
+
+bool
+ListAggregatorModel::removeRows(int row, int count, const QModelIndex& parent)
+{
+    if (row < 0 || row >= rowCount() || count <= 0) {
+        return false;
+    }
+ 
+    /* Note that since this is an aggregator, the underlying models will
+       take care of signaling the addition and removal of rows and this
+       model will update accordingly.
+       Therefore we don't need to call beginRemoveRows */
+
+    int removed = 0;
+    Q_FOREACH (QAbstractItemModel* model, m_models) {
+        /* Please note that the offset of the current model is computed
+           by iterating over all previous models and making a sum of their
+           row count.
+           By taking that into account the calculation for removeAt is:
+           (row + removed) - (offset + removed)
+           This can be simplified to just row - offset as you see below */
+        int offset = computeOffset(model);
+        int removeAt = row - offset;
+
+        if (removeAt < 0 || removeAt >= model->rowCount()) {
+            // The item(s) that we need to remove are outside of this model
+            // move on to the next one.
+            continue;
+        }
+        
+        int removeCount = qMin(count - removed, model->rowCount() - removeAt);
+        model->removeRows(removeAt, removeCount);
+        removed += removeCount;
+
+        if (removed >= count) {
+            break;
+        }
+    }
+
+    return true;
+}

=== modified file 'libunity-2d-private/Unity2d/listaggregatormodel.h'
--- libunity-2d-private/Unity2d/listaggregatormodel.h	2011-06-07 16:34:46 +0000
+++ libunity-2d-private/Unity2d/listaggregatormodel.h	2011-06-09 15:57:26 +0000
@@ -44,6 +44,8 @@
     QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const;
     Q_INVOKABLE int rowCount(const QModelIndex& parent = QModelIndex()) const;
     Q_INVOKABLE QVariant get(int row) const;
+    Q_INVOKABLE virtual bool removeRows(int row, int count,
+                                        const QModelIndex& parent = QModelIndex());
 
     /* This method is the QML equivalent of aggregateListModel.
        The reason why aggregateListModel wasn't directly exposed to QML is that

=== modified file 'libunity-2d-private/Unity2d/windowslist.cpp'
--- libunity-2d-private/Unity2d/windowslist.cpp	2011-04-28 13:09:25 +0000
+++ libunity-2d-private/Unity2d/windowslist.cpp	2011-06-09 15:57:26 +0000
@@ -17,6 +17,7 @@
 #include <QRegExp>
 #include <QApplication>
 #include <QWidget>
+#include <QList>
 
 #include <debug_p.h>
 #include "windowslist.h"
@@ -210,3 +211,20 @@
         }
     }
 }
+
+bool WindowsList::removeRows(int row, int count, const QModelIndex& parent)
+{
+    if (row < 0 || row >= m_windows.count() || count <= 0) {
+        return false;
+    }
+    count = qMin(count, m_windows.count() - row);
+
+    beginRemoveRows(parent, row, row + count - 1);
+
+    for (int i = 0; i < count; i++) {
+        m_windows.removeAt(row);
+    }
+
+    endRemoveRows();
+    return true;
+}

=== modified file 'libunity-2d-private/Unity2d/windowslist.h'
--- libunity-2d-private/Unity2d/windowslist.h	2011-02-08 11:48:16 +0000
+++ libunity-2d-private/Unity2d/windowslist.h	2011-06-09 15:57:26 +0000
@@ -37,6 +37,8 @@
 
     QVariant data(const QModelIndex & index, int role = Qt::DisplayRole) const;
     int rowCount(const QModelIndex & parent = QModelIndex()) const;
+    Q_INVOKABLE virtual bool removeRows(int row, int count,
+                                        const QModelIndex& parent = QModelIndex());
 
     Q_INVOKABLE void load();
     Q_INVOKABLE void unload();
@@ -46,7 +48,7 @@
     void removeWindow(BamfView *view);
     void updateWorkspaceRole(int workspace);
 
-private:
+protected:
     QList<WindowInfo*> m_windows;
 };