Reviewing of GroupsModel

[Gustavo Pichorim Boiko]

Hi

As I have promised, I did a first round of reviewing of the GroupsModel
classes. Here are my findings:

groups-model.h:47: roles
    Maybe we could add a AccountsModel::LastRole or something like that
    to indicate the last custom role added by that class. For usage in
    QML, the roles must not have conflicting numbers.

groups-model.h:67-68: void addContactToGroups();
    This should probably be renamed to addContactToGroup, since it adds
    the contact to just one group at a time.

groups-model.cpp:54: GroupsModel::GroupsModel()
    For usage in QML, it needs to use the role names from the
    AccountsModel so that delegates can use it

groups-model.cpp:173: void GroupsModel::onItemsAdded()
    Maybe this one should be renamed to addItems() as (semantically) it
    is what it does. See also the comments on groups-model-item

groups-model.cpp:211: void GroupsModel::onSourceItemsRemoved()
    Implementation missing

groups-model.cpp:216: void GroupsModel::loadAccountsModel()
    Before moving to tp-qt4-yell, it would be better to add the
    GroupsModel as a friend class of the accounts model to access the
    node list directly instead of doing many index() and data() calls.

groups-model.cpp:*:
    The concept of ungrouped contacts could probably be revisited or
    changed. Maybe it would be better to handle the visualization of
    ungrouped contacts in the app? I'm just not sure how to represent
    that in the model in a flexible way

groups-model-item.cpp:94: void GroupsModelItem::setGroupName()
    Needs to notify the change (the model needs to emit the dataChanged
    signal in this case). Maybe it should also rename the group in the
    contacts' group list?

groups-model-item.cpp:104: void
GroupsModelItem::addProxyContact(ProxyTreeNode *proxyNode)
    In AccountsModel, the children nodes are added inside this function,
    and then the signal is emitted. Maybe we should do it this way here
    too so that we keep consistent.

proxy-tree-node.h:*: class naming
    We have two options here: either we make the proxy-tree-node a real
    generic proxy for TreeNodes (not for contact items), and add a
    ProxyContactItem that inherits from the ProxyTreeNode, or we rename
    ProxyTreeNode to reflect its content and purpose.

proxy-tree-node.cpp:74: bool ProxyTreeNode::setData()
    Instead of just returning false here, it should probably act like a
    real proxy and call setData in the source item, returning its return
    value.

Cheers

Boiko