[Bug 31195] Implement models

[bugzilla-daemon at freedesktop.org]

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

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ollisal at gmail.com

--- Comment #10 from Olli Salli <ollisal at gmail.com> 2011-01-11 04:22:49 PST ---
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.

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