This is a follow up of the discussion started in http://lists.freedesktop.org/archives/telepathy/2010-October/004994.html.
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.
Thanks for the detailed review. The code you are referring to is located here: http://git.collabora.co.uk/?p=user/paolo/telepathy-qt4.git;a=summary The latest developments are in the 'contacts' branch. As you can see, I removed all calls to becomeReady and other telepathy initialization functions. The models now assume that the objects passed to it are already initialized. To avoid duplicating a lot of the bookkeeping of the accounts model, I also merged contacts and accounts in a single hierarchical model (where accounts are at level 1, and contacts for each account at level 2). You can also use this model if you only need the accounts, and a flag could be added to save the overhead involved in maintaining contact data for this use case. If you only need an aggregated contact list, you can still use this model, and wrap it in a FlatModelProxy, which will flatten it out removing first level nodes.
Ok, so follows a full review from me as well (leaving aside models since Andre was already picky enough on that). I mostly agree with what was written (with the obvious reserve of what I wrote in my very first mail), however there are some points which are not really clear to me. First of all the main question should be: what do we want to provide to client applications written in QML? Considering a wide set of use cases, I'd say: Contact list; very basic presence settings; tubes; file transfers. For example, I don't see the need of exposing text message features. Considering use cases of our two main client platforms (KDE and Maemo/Meego), it makes no sense for an application to start a text chat - the main channel handler would take care of that. If there are plans for writing the CH for Text Channels for Meego in QML, the component could be moved there - but as for myself, I don't see the need of exposing this component through QML. Instead, I find it quite critical to expose Tubes and File Transfers - which are exactly the reason why a 3rd party app could be tempted to use Telepathy - and for your pleasure, also the most tricky bits to implement :) So basically I'd like to start a discussion about "what do we want to expose?", because (apart from the most obvious ContactList and Account) this seems to me the most critical part. And, if you agree with what I said, I guess we also need to discuss how.
(In reply to comment #3) > I mostly agree with what was written (with the obvious reserve of what I wrote > in my very first mail), however there are some points which are not really > clear to me. You mean the issue with reference counting? Maybe I'm missing something, but I don't see the problem there. At the moment, reference counted objects are exposed through qml-friendy wrappers, which keep the object alive internally. Unless you store the object on the QML-side so that it outlives its container, you shouldn't be concerned with the reference count at all. Of course, it is always possible to delete the object in C++ code, and later access it in QML (I believe you would get 'undefined' in this case), but that is nothing specific to telepathy, and has simply to do with how object handles are implemented in QML. > For example, I don't see the need of exposing text message features. > Considering use cases of our two main client platforms (KDE and Maemo/Meego), > it makes no sense for an application to start a text chat - the main channel > handler would take care of that. If there are plans for writing the CH for Text > Channels for Meego in QML, the component could be moved there - but as for > myself, I don't see the need of exposing this component through QML. I don't know about Meego, but I believe there is interest in experimenting with a QML chat handler for KDE. In any case, these models are not just for QML: I think the kde chat handler could possibly benefit from them. > Instead, I find it quite critical to expose Tubes and File Transfers - which > are exactly the reason why a 3rd party app could be tempted to use Telepathy - > and for your pleasure, also the most tricky bits to implement :) > > So basically I'd like to start a discussion about "what do we want to expose?", > because (apart from the most obvious ContactList and Account) this seems to me > the most critical part. And, if you agree with what I said, I guess we also > need to discuss how. Yes, I agree that tubes and file transfers are interesting and could be wrapped in a high-level API geared towards QML. At the moment I'm mostly focusing on models, and I'd like to polish them and have them merged, before moving on to more challenging stuff, but any suggestion on how those interfaces should look like is of course welcome.
This bug has people actively working on it; thus changing state.
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.
One more thing I forgot here. You may want to decouple AccountsModelItem from ContactModelItem in such a way that custom AccountsModelItem could be created with custom ContactModelItems so applications fetching contacts from outside tp-qt4 (nepomuk, tracker) could make use of the models. Maybe adding a virtual loadContacts that could be overridden by custom AccountsModelItem and also a virtual loadAccount in AccountsModel to be able to create custom AccountsModelItem. Something along those lines.
OK, most of the changes have been done, check http://git.collabora.co.uk/?p=user/mbatle/telepathy-qt4.git;a=shortlog;h=refs/heads/review1 Start review from commit b292753a30759c3656a693a1ce41bf5f0ecb2090 Things missing: - 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. Some answers to your questions: > +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. setData is a standard function of models, so if we want to represent the type, status and status message as roles, then we should allow setting them individually in despite of the worse performance. > + 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. This can be done, although in most cases it would not provide any improvement IMHO. > +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? Yes, that is used in several places in our client, in case of connection status changed is very different than from a change in some properties. It is used for example to become ready the connection, upgrade the contacts, etc.
setStatus and setStatusMessage are not exposed anymore, check commit d756293c949c2b52e14360a075adc99e67f4e84a in review1 branch
The first part of my review for the status of the models at andre's commit 5d3de777... follows. This is mostly based on the API; I'll take a quick look at the implementation later. I've mentioned this in brief to Andre earlier, but anyway: As it seems you want AccountsModel to be a simple set of accounts, and prospectively want to filter that, I believe AccountsModel should wrap Tp::AccountSet instead of Tp::AccountManager. AccountSet is a (filtered sub)set of an AccountManager's accounts with change notification on accounts being added or removed to/from the set (either because accounts are added/removed completely, or because their attributes changed which affects if they match that set's filter). Note that an empty filter is also valid, which means all accounts, if somebody wants to use that. AccountSet doesn't offer access to creating new accounts, but neither does your proposed model, so that's not a problem. I think this is a reasonable approach; only the account editing UI in a given platform needs that and that part doesn't need to be written using models. Whether contacts are in the same model as the accounts or not, note that accounts have capabilities too. So capabilities aren't contact-only roles. Account capabilities indicate whether a given account can be used for e.g. video calls with ANY contact (whether you actually have a contact who is capable of receiving calls on that account or not). The availability of a given capability depends on the features the protocol offers, whether the CM implements them or not, and service-specific limitations (e.g. GTalk is a service using the Jabber protocol, and doesn't support everything some other Jabber service supports). Tp::Account handles all of this transparently for both online and offline accounts. If you're exposing the protocol and CM name, you should also expose the service name so e.g. the aforementioned GTalk accounts can be distinguished from other jabber accounts. Beware that Tp::Account::serviceName() reverts to something autogenerated from the protocol (currently "im-<protocol>"), if no specific service is configured. As a quick implementation note: making setting a contact's publish state to No (stopping to send presence to them) completely remove the contact (by calling removeContacts() on them) is quite harsh, and not that intuitive either. Similarly, automatically requesting somebody's presence only because we accepted their request to see ours assumes too much - it's application-specific whether you actually want to do this, or not. Text message sending totally disregards the type and flags for a message. I don't know how well overloads and/or default parameters could be used for QML / Q_INVOKABLE methods - so you might need multiple variants. The flags currently specify different variants of delivery reporting, but might include more details in the future, and the other message types besides NORMAL are used widely on many protocols, especially IRC. Again, the models layer for tp-qt4 is in no better position to assume some particular (APPLICATION SPECIFIC) value for a parameter than tp-qt4 itself, or the CM behind D-Bus. I'd much rather see the conversation model using the same Contact objects as the rest of the API rather than just exposing a contact's alias and avatar. This means the conversation model would need to be a ItemModel too, right? Or at least expose some other details of the contact too, if the current approach is significantly more convenient - many text chat UIs display an icon for the contact's presence beside received messages, for example, especially for multi-user chatrooms.
Additional bits about the implementation (Telepathy usage only. I trust you in getting the actual qt model implementation stuff right better than I ever would notice with my limited knowledge): 50 void AccountsModelItem::Private::setStatus(const QString &value) 51 { 52 Presence presence = mAccount->currentPresence().barePresence(); 53 presence.setStatus(Tp::ConnectionPresenceTypeUnset, value, QString()); 54 mAccount->setRequestedPresence(presence); 55 } 56 57 void AccountsModelItem::Private::setStatusMessage(const QString &value) 58 { 59 Presence presence = mAccount->currentPresence().barePresence(); 60 presence.setStatus(Tp::ConnectionPresenceTypeUnset, QString(), value); 61 mAccount->setRequestedPresence(presence); 62 } 63 From the Telepathy D-Bus interface specification for Account.RequestedPresence (which setRequestedPresence manipulates): "The Connection_Presence_Type in this property MUST NOT be Unset, Unknown or Error.". So, you're working against the spec here. The spec also doesn't guarantee, or even hint that an unset/invalid value in one of the fields would mean keeping the current value. If you see such behavior currently, that's just an implementation artifact of MC (or whatever you're using there). Additionally, as Andre pointed out, setting both the status and the message at the same time would be more sensible, and not only for performance reasons: on many protocols, only a subset of all the available statuses support a message. So, one would need for the AM and CM to process the status change request, and make a round-trip to the IM server to set the presence, and the stack to pick up the change before you could actually set the message. (Also, the fact that setStatus currently secretly clears the message seems error-prone). Unfortunately, correctly setting presence for all accounts would require implementing http://telepathy.freedesktop.org/spec/Protocol_Interface_Presence.html (in CMs, some data files, and tp-qt4). That's definitely a TODO item for us: we'll implement a Tp::Account::allowedPresenceStatuses() which uses info from a connected connection if any, the service-specific profile file and that D-Bus interface (and the corresponding serialized information in data files). Andre says this is possible to accomplish sometime next week. Why are implementation helper functions like AccountsModelItem::clearContacts and addKnownContacts public? What's the rationale for (dis)connectChannelQueue()?
Why are implementation helper functions like AccountsModelItem::clearContacts and addKnownContacts public? This was implemented this way to force a reset of the model in some situations, due to some shortcomings of the Qt MVC model. Specifically, logging an account an account in and out was not refreshing the model properly. It can probably be implemented internally now that 4.7.1 is out. What's the rationale for (dis)connectChannelQueue()? This was done for those case where you don't want the messageQueue to be acknowledged automatically. Say, you open a chat, ack all pending messages, then hide it, and you don't want the messages to be ack'd automatically until you show the chat message again. As for your other comments, I agree. Particularly the ones referring to the status message. It's not working ok anyway, so we might as well follow the API. ;)
Hi, I've been working with kde-telepathy group to bring a working contact list based on models from Telepathy-Qt4-Yell project. We did some modifications to the AccountsModel with the (tree) view in mind. I've created a repo on github, which includes copy of tp-qt4-yell master with our applied changes. We did mainly 3 things: 1) Don't add accounts that are disabled or offline. Imagine this in the view - you add all available accounts, that creates one tree node for every account and those accounts that are disabled would be in the view for no reason. Same with the offline accounts, because the group would be empty anyway (there are no childs in this tree node until you actually connect), therefore it has no use in the view. 2) When an account goes offline in the current implementation, all the contacts of the account node are deleted, which leaves the account node with no childrens, ie. empty group. So we remove the account node as well (in fact, we remove only this node, all the children are deleted in the process). With this comes also one fix in tree-node.cpp, when you go offline and online several times, sooner or later it will crash because it tries to access already deleted pointer. We moved the "mPriv->mParent->mPriv->mChildren.removeOne(this);" line from destructor to the remove() method, which seems to fix the crashing. 3) There is one new method, AccountsModel::accountForContactIndex(const QModelIndex& index). This returns the account where the contact at index index belongs. This is needed when requesting text/audio/video channel, as you need to call this on an account object with the contact as a parameter. We'll continue to improve the model (implement groupping) and share all the changes at github (we'll try to separate the added functionality by commits with some descriptive commit message) so it can be eventuelly merged back. Here's the repo: https://github.com/mck182/telepathy-qt4-yell Feel free to ask about/comment whatever pops up there. Marty (mck182 @ #kde-telepathy)
Expanding on Martin's comments 2) The bug in number 2 is a bug in the model, currently if someone deletes a row (any row, account or contact) and then looks at the model again before hitting the event loop the model size and data were out of sync and it could crash. The cause of this is that we call startRemoveRows() - then remove() on the TreeItem, then endRemoveRows(). The method remove() in TreeItem, doesn't update the parents list of row information. Instead it calls deleteLater(). The code to remove the row from the parent happens in the destructor. So there is a short time when it's all out of sync This became prominent and caused crashes because of the earlier change Martin made at 1, however it could happen at other points. There is also another quite important fix. 4) In the model in Tp-yell when you start the model but are already online no contacts are shown. Contacts are only listed when the connection status changed. To resolve this, addKnownContacts() is called in the constructor of the account item. I had to use a single shot timer to do this as without it it starts emitting signals (new contact) before the parent code can connect to it.
(In reply to comment #13) (...) > > 1) Don't add accounts that are disabled or offline. Imagine this in the view - > you add all available accounts, that creates one tree node for every account > and those accounts that are disabled would be in the view for no reason. Same > with the offline accounts, because the group would be empty anyway (there are > no childs in this tree node until you actually connect), therefore it has no > use in the view. Keep in mind that this model is not used just by KDE, so there might be more use cases than just the KDE ones. For instance, the accounts model can be used (the toplevel items) in the kcm account for example, and there you really want to display accounts that are offline and/or disabled. Also, such accounts can be easily filtered out by adding a simple sortfilterproxymodel on top of the existing model. I see really no point in changing the original models to achieve that. > 2) When an account goes offline in the current implementation, all the contacts > of the account node are deleted, which leaves the account node with no > childrens, ie. empty group. So we remove the account node as well (in fact, we > remove only this node, all the children are deleted in the process). With this > comes also one fix in tree-node.cpp, when you go offline and online several > times, sooner or later it will crash because it tries to access already deleted > pointer. We moved the "mPriv->mParent->mPriv->mChildren.removeOne(this);" line > from destructor to the remove() method, which seems to fix the crashing. Well, same point as the first one, I don't think removing this from the model is a good idea. > 3) There is one new method, AccountsModel::accountForContactIndex(const > QModelIndex& index). This returns the account where the contact at index index > belongs. This is needed when requesting text/audio/video channel, as you need > to call this on an account object with the contact as a parameter. This looks fine (the idea, haven't checked the implementation though). > We'll continue to improve the model (implement groupping) and share all the > changes at github (we'll try to separate the added functionality by commits > with some descriptive commit message) so it can be eventuelly merged back. > > Here's the repo: https://github.com/mck182/telepathy-qt4-yell > > Feel free to ask about/comment whatever pops up there. In any case, it is really nice that you are using/contributing back to the models in tp-qt4-yell, thanks a lot.
(In reply to comment #15) > > Keep in mind that this model is not used just by KDE, so there might be more > use cases than just the KDE ones. For instance, the accounts model can be used > (the toplevel items) in the kcm account for example, and there you really want > to display accounts that are offline and/or disabled. > > Also, such accounts can be easily filtered out by adding a simple > sortfilterproxymodel on top of the existing model. I see really no point in > changing the original models to achieve that. True. So we reverted the filtering back and moved it to our proxy filter model, which is not part of the repo as it is very application specific and thus not useful for this project (but if you want, I can add it...). > > > 3) There is one new method, AccountsModel::accountForContactIndex(const > > QModelIndex& index). This returns the account where the contact at index index > > belongs. This is needed when requesting text/audio/video channel, as you need > > to call this on an account object with the contact as a parameter. > > This looks fine (the idea, haven't checked the implementation though). We added another one, AccountsModel::contactForIndex(const QModelIndex& index), which returns Tp::ContactPtr, so that it can be easily retrieved for the purposes of context menu/contact properties popup in the view. Again, here's the repo: https://github.com/mck182/telepathy-qt4-yell
Oh and I forgot - David Edmundson had yet another great find - in AccountsModelItem::onStatusChanged(..) it's great to call onChanged(), since this would trigger the proxy model filters and thanks to that we can filter out accounts from the view that just went offline. Or let in the accounts that just came online :)
Most of the issues have been fixed. We are now working on the unit tests. When we are done with that, we'll merge all changes into master and we'd need a new review to verify it's ok to merge the models into main tp-qt4
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-qt/issues/11.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.