[Bug 31195] Implement models

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=31195

--- Comment #1 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-10-28 06:21:50 PDT ---
Hey, so finally had some time to check this, sorry the delay:

So here goes a brief summary of how I think models should be done in tp-qt4,
comments on the code itself follows below:

Note that some of the comments already apply to the current code.

First I believe models should expose objects everywhere that this is possible, 
allowing models code to be more easily maintained (by splitting different logic 
in different objects) and also by allowing Q_PROPERTY notification system to 
work easily, so you don't need to use dataChanged whenever something in a 
specific object changed.

Also models should be added either to TelepathyQt4/models or TelepathyQt4 
itself (I would prefer /models for now and we can change later once we advance 
on this). Right now they are placed in TelepathyQt4/ui, but the code is not UI
specific and does not even depend on QtGui.

Accounts:
- The model for accounts should be a simple model exposing AccountModelItem 
QObjects. This object would wrap Tp::Account and properly expose the properties 
(using Q_PROPERTY) that make sense.

  So there would be an AccountsModel (not AccountModel) that has 1 role named 
AccountRole that would return an AccountModelItem. It would only change when 
accounts are added/removed, all the rest should be done trough change 
notification in the AccountModelItem object.

  The AccountModelItem object would keep a ref to the underlying Tp::Account 
object and not expose it to avoid issues with shared pointer.

  The model should not call becomeReady() on any object, the user should pass a 
proper AM object with the factories that make sense for his usage. Also do not 
assert if the am is not ready, just warn and do nothing and properly document 
it.

   Maybe you want to receive a filter in the model constructor to filter the 
accounts you are interested in, using AM::filterAccounts. You may be interested 
only in enabled accounts for example.

Contacts:
- Despite the fact that most applications will use their own model for
contacts,
that would query nepomuk or tracker, or wathever, I think it is useful to have
a model for contacts that would work for tp-qt4 contacts, for applications 
using tp-qt4 only for example.

  So first I would name it ContactsModel instead of ContactListModel. This
model should have one role returning a ContactModelItem wrapping a Tp::Contact
the same way as explained above for the accounts model.

  This ContactModelItem could be used later for the text channel model, as you
may be interested in the contact changes when chatting with a contact, more on 
this later. 

  Same as above you should not call becomeReady on any account here or contacts
(for now you need it for contacts, but ContactFactory is being developed and 
should be used instead), so just use upgradeContacts for now and add a comment
to remove it once ContactFactory is in place.

  I am not sure if we should have one model per account or one model for all 
accounts. I believe one model for all account is the best approach here, as it 
is easier to show a contact list with all contacts from all accounts this way.
So you should do the same as done above, receive an AM object as params and a 
filter, to be able to filter the accounts you are interested.

TextChannel:
- Having a model for all kinds of channel aggregated does not make sense, as 
they are completelly different, so what you want here is a model for each kind
of channel (TextChannelModel, FileTransferChannelModel, etc). 

  The model should wrap one channel only and expose the contacts in the channel
using ContactModelItem (so you have change notification for free).

Note that no model should expose SharedPtr directly, they should expose a 
QObject instead that keeps a ref for the object instead.

Now to the code.

First and really important, stick to HACKING. I see various places where the 
coding style is not being followed, and all tp-qt4 code should follow the same 
coding style, examples:

--------------
- Extra spaces in the end of line
- Extra lines, always use 1 line to separate methods, blocks, etc

--------------

Coding style issues:

--------------
+#include <TelepathyQt4/ui/AccountModel>
+#include "TelepathyQt4/ui/_gen/account-model.moc.hpp"
+#include <TelepathyQt4/PendingReady>

See other classes, add spaces between the main include and the moc include,
also
add a space from the moc include and all the other includes. Also all extra 
includes should be ordered alphabetically and the tp-qt4 specific includes
should be added before any system include (Qt or wathever)

--------------
+AccountModel::AccountModel(const Tp::AccountManagerPtr &am, QObject *parent)
+    : QAbstractListModel(parent)
+    , mAM(am)

The , should be after (parent), check other classes.

--------------

+namespace Tp
+{
+    ContactsListModel::ContactsListModel(const Tp::AccountManagerPtr &am,
QObject *parent)
+     : QAbstractListModel(parent),
+     mAM(am)

Extra indentation

--------------
+        //add items to model

Always add a space after // and /*

--------------
+        if (index.row() >= mContacts.count())
+            return QVariant();
Always add {} even for one-liners statements

--------------
ui/examples
The examples should be inside the top-level examples directory.

--------------
+#include "TelepathyQt4/ui/examples/conversation/_gen/chatwindow.moc.hpp"
Just use _gen/foo.h for examples.

--------------
+public:

+    explicit AccountModel(const Tp::AccountManagerPtr &am, QObject *parent =
0);
There should be no extra space after public.

Now other small issues with the code:
--------------
+void AccountModel::onAccountRemoved()
+{
+    Account *account = qobject_cast<Account *>(sender());
Do not use sender(), it won't be needed if you follows the approach I explained 
above.

+    qWarning() << "Received change notification from unknown account";
What is with this warning?

--------------
+        case ConnectionRole:
+            return account->connectionObjectPath();
We probably don't want/need to expose the connection object path to the user, 
also this method is deprecated and will be removed in the next release.

--------------
+        case RequestedStatusMessage:
+        case ConnectionManagerRole:
Use RequestedPresenceStatusMessage and ConnectionManagerNameRole here, 
you probably don't need it in case we follow what I described above, but
anyway.

--------------
+    mAccounts[row]->setEnabled(value);
+    emit dataChanged(index(row), index(row));

same for 

+    account->setRequestedPresence(presence);
+    emit dataChanged(index(row), index(row));

and

+    mAccounts[row]->setNickname(value);
+    emit dataChanged(index(row), index(row));
You should not emit dataChanged when you call a async method, you should wait 
for the change notification signal first.

--------------
For the account item, you probably want to expose as much info as possible,
for example Account::changingPresence, so you know when the presence change 
process starts/ends.

We are in 2010 and the code is new so, use copyright 2010 only.

That is it for now

We should discuss more about this later, probably an IRC meeting, let's try 
to setup one.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.