[Bug 31195] Implement models
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Wed Dec 22 14:50:48 CET 2010
https://bugs.freedesktop.org/show_bug.cgi?id=31195
--- Comment #6 from Andre Moreira Magalhaes <andrunko at gmail.com> 2010-12-22 05:50:47 PST ---
Here goes another review round. The changes are quite good and I believe it's
in a much better state now than before, but still needs some tweaks here and
there.
The review is based on the master branch from:
http://git.collabora.co.uk/?p=user/boiko/telepathy-qt4.git;a=summary
General notes:
- Add a pretty header for all public headers, such as contact-model-item.h,
etc.
- Add #ifndef IN_TELEPATHY_QT4_HEADER ... in all public headers.
- Move all private members to a Private class. Public classes should not have
private members other than a mPriv pointer for API/ABI stability reasons. The
same applies to helper methods, they should all go into the Private class and
only slots should be declared in the main class.
- Use debug from debug-internal.h instead of qDebug.
- Stick to the tp-qt4 coding style, check HACKING and other classes, specially
empty spaces in the end of line.
- Normalize all signal connections, for example:
connect(foo, SIGNAL(bar(Foo *)), ...) -> connect(foo, SIGNAL(bar(Foo*)), ...)
- Add tests for all new classes, the test coverage (make lcov-check) should be
at least yellow.
- Make sure the examples won't break built if QtDeclarative is not installed.
On TelepathyQt4/models/AccountsModel
+#ifndef _TelepathyQt4_ui_AccountModel_HEADER_GUARD_
+#define _TelepathyQt4_ui_AccountModel_HEADER_GUARD_
Please use the same header name for the header guard, AccountsModel
(plural) instead of AccountModel, also s/ui/models/
On TelepathyQt4/models/AvatarImageProvider
+#ifndef _TelepathyQt4_AvatarImageProvider_HEADER_GUARD_
+#define _TelepathyQt4_AvatarImageProvider_HEADER_GUARD_
Add _models_ before AvatarImageProvider
On TelepathyQt4/models/ConversationModel
+#ifndef _TelepathyQt4_ui_ConversationModel_HEADER_GUARD_
+#define _TelepathyQt4_ui_ConversationModel_HEADER_GUARD_
s/ui/model/
On TelepathyQt4/models/FlatModelProxy
+#ifndef _TelepathyQt4_FlatModelProxy_HEADER_GUARD_
+#define _TelepathyQt4_FlatModelProxy_HEADER_GUARD_
Add _models_ before FlatModelProxy
On TelepathyQt4/models/accounts-model-item.cpp
+#include "TelepathyQt4/models/contact-model-item.h"
Include the pretty header ContactModelItem instead of contact-model-item.h.
+namespace Tp
+{
+
+AccountsModelItem::AccountsModelItem(const AccountPtr &account)
+ : mAccount(account)
+{
+ if (!mAccount->connection().isNull()) {
+ ContactManagerPtr manager = account->connection()->contactManager();
+ foreach (ContactPtr contact, manager->allKnownContacts()) {
+ addChild(new ContactModelItem(contact));
+ }
+
+ connect(manager.data(),
+ SIGNAL(allKnownContactsChanged(Tp::Contacts, Tp::Contacts,
+
Tp::Channel::GroupMemberChangeDetails)),
+ SLOT(onContactsChanged(Tp::Contacts, Tp::Contacts)));
+ }
Please refactor refreshKnownContacts into 2 methods, clearContacts and
addKnownContacts and call addKnownContacts here. As the model keeps track of
contact changes, there is no point in making refreshKnownContacts invokable.
+ connect(mAccount.data(),
+ SIGNAL(removed()),
+ SLOT(onRemoved()));
...
+ connect(mAccount.data(),
+ SIGNAL(connectionChanged(const Tp::ConnectionPtr&)),
+ SLOT(onChanged()));
+ connect(mAccount.data(),
+ SIGNAL(connectionChanged(const Tp::ConnectionPtr&)),
+ SLOT(onConnectionChanged(const Tp::ConnectionPtr&)));
Please check for duplicate connections here, such as connectionChanged that is
being connected twice. If you want to call 2 slots from a signal emission, just
connect to one of them and call the other one from there.
+QVariant AccountsModelItem::data(int role) const
+{
+ switch (role) {
...
+ case AccountsModel::AutomaticPresenceRole:
+ return mAccount->automaticPresence().status();
+ case AccountsModel::CurrentPresenceRole:
+ return mAccount->currentPresence().status();
+ case AccountsModel::CurrentPresenceTypeRole:
+ return mAccount->currentPresence().type();
+ case AccountsModel::CurrentPresenceStatusMessageRole:
+ return mAccount->currentPresence().statusMessage();
+ case AccountsModel::RequestedPresenceRole:
+ return mAccount->requestedPresence().status();
+ case AccountsModel::RequestedPresenceTypeRole:
+ return mAccount->requestedPresence().type();
+ case AccountsModel::RequestedPresenceStatusMessageRole:
+ return mAccount->requestedPresence().statusMessage();
...
For consistency add roles for presence type/status/statusMessage for
all of automatic/current/requestedPresence.
+bool AccountsModelItem::setData(int role, const QVariant &value)
+{
+ switch (role) {
+ case AccountsModel::EnabledRole:
+ setEnabled(value.toBool());
+ return true;
+ case AccountsModel::RequestedPresenceRole:
+ setStatus(value.toString());
+ return true;
+ case AccountsModel::RequestedPresenceStatusMessageRole:
+ setStatusMessage(value.toString());
+ return true;
It would be good to be able to set the presence status and status message at
the same time,
avoiding 2 dbus calls when someone wants to set both at the same time.
+void AccountsModelItem::setStatus(const QString &value)
+{
+ SimplePresence presence = mAccount->currentPresence().barePresence();
+ presence.status = value;
+ mAccount->setRequestedPresence(presence);
Use Presence instead of SimplePresence
+}
+
+void AccountsModelItem::setStatusMessage(const QString& value)
+{
+ SimplePresence presence = mAccount->currentPresence().barePresence();
+ presence.statusMessage = value;
+ mAccount->setRequestedPresence(presence);
Same here
+void AccountsModelItem::onRemoved()
+{
+ int index = parent()->indexOf(this);
+ emit childrenRemoved(parent(), index, index);
Make sure this won't leak here if no one is deleting it. Make sure
AccountsModel will delete it.
+void AccountsModelItem::onContactsChanged(const Tp::Contacts &addedContacts,
+ const Tp::Contacts &removedContacts)
Coding style, no need to align (which is not the case) params. Use:
void AccountsModelItem::onContactsChanged(const Tp::Contacts &addedContacts,
const Tp::Contacts &removedContacts)
+
+void AccountsModelItem::onConnectionChanged(const Tp::ConnectionPtr
&connection)
+{
+ //if the connection is invalid or disconnected, clear the contacts list
+ if (connection.isNull()
+ || !connection->isValid()
+ || connection->status() == ConnectionStatusDisconnected) {
+ emit childrenRemoved(this, 0, mChildren.size() - 1);
+ return;
+ }
+
+ ContactManagerPtr manager = connection->contactManager();
+
+ connect(manager.data(),
+ SIGNAL(allKnownContactsChanged(Tp::Contacts, Tp::Contacts,
+
Tp::Channel::GroupMemberChangeDetails)),
+ SLOT(onContactsChanged(Tp::Contacts, Tp::Contacts)));
+}
Call clearContacts and addKnownContacts here.
+void AccountsModelItem::refreshKnownContacts()
+{
+ //reload the known contacts if it has a connection
space after //
+ QList<TreeNode *> newNodes;
+ if (!mAccount->connection().isNull()
+ && mAccount->connection()->isValid()) {
&& in the first line and the second line aligned with the opening ( like:
+ if (!mAccount->connection().isNull() &&
+ mAccount->connection()->isValid()) {
+ //remove the items no longer present
space after //
+ for (int i = 0; i < mChildren.size(); ++i) {
Cache children.size here.
+ bool exists = false;
+ ContactModelItem *item = qobject_cast<ContactModelItem
*>(childAt(i));
+ if(item) {
space after if and before (
+ ContactPtr itemContact = item->contact();
+ if(contacts.contains(itemContact)) {
Same here
+ exists = true;
+ break;
+ }
+ }
+ if(!exists) {
And here
...
On TelepathyQt4/models/accounts-model-item.h
+#ifndef _TelepathyQt4_account_model_item_h_HEADER_GUARD_
+#define _TelepathyQt4_account_model_item_h_HEADER_GUARD_
Add _models_ before _account_model_ here
And Add:
#ifndef IN_TELEPATHY_QT4_HEADER
#error IN_TELEPATHY_QT4_HEADER
#endif
+#include "tree-node.h"
#include <TelepathyQt4/models/TreeNode>
...
+ Q_INVOKABLE virtual QVariant data(int role) const;
+ virtual bool setData(int role, const QVariant &value);
+ Q_INVOKABLE AccountPtr account() const { return mAccount; }
+
+ void setEnabled(bool value);
+
+ Q_INVOKABLE void setStatus(const QString &value);
+
+ Q_INVOKABLE void setStatusMessage(const QString& value);
+
...
+
+ Q_INVOKABLE void setPresence(int type, const QString &status, const
QString &statusMessage);
I would avoid exposing setStatus and setStatusMessage in favor of only exposing
setPresence or better phrased setCurrentPresence.
+ Q_INVOKABLE void refreshKnownContacts(void);
As I said before, I don't see a point in exposing this method as the model
keeps track of contacts itself.
+
+Q_SIGNALS:
+ void connectionStatusChanged(const QString &accountId, int status);
What is the point in having this signal? Shouldn't it be emitted as changed()
as the other properties do? Any special case here?
On TelepathyQt4/models/accounts-model.cpp
+#include <TelepathyQt4/models/AccountsModel>
+
+#include "TelepathyQt4/models/_gen/accounts-model.moc.hpp"
+
+#include <TelepathyQt4/PendingReady>
+#include <TelepathyQt4/ContactManager>
+
+#include "TelepathyQt4/models/accounts-model-item.h"
+#include "TelepathyQt4/models/contact-model-item.h"
Use the pretty headers here.
That is for now, I will get back to it once the issues are fixed.
I would like to ask you to also rebase the branch against upstream master to
make it easier to review.
--
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
More information about the telepathy-bugs
mailing list