[Bug 29090] Add support to filter accounts by RequestableChannelClasses

[bugzilla-daemon at freedesktop.org]

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

Olli Salli <ollisal at gmail.com> changed:

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

--- Comment #4 from Olli Salli <ollisal at gmail.com> 2010-08-06 10:45:52 PDT ---
I don't like the fact that AM now auto-enables ProtocolInfo. This causes loads
of extra initialization (O(number-of-accounts) disk accesses and extra D-Bus
round trips when introspecting the Accounts) whether you want to filter by caps
or not. I'd add an AM optional feature FilterByCapabilities, which enables
ProtocolInfo in the Accounts, and document the supports*AccountsSet() methods
and filterAccounts() with rccSubset as requiring that (and invalidate the
filter if not enabled).

Actually, the supports*AccountsSet() methods could use filterAccounts() in the
first place for clarity (no big deal though as the method is trivial).

That said, I find stashing the channel classes to the same variant map as the
actual account properties a bit confusing API-wise, especially as it's just
documented as an "additional allowed key", not quite indicating how the way it
affects the filtering differs from the other keys! As an implementation detail
it's okay though - but to make the API clearer I'd do the following:
 - add overload for filterAccounts accepting just a RCCList - makes it easier
to implement filter functions for application-specific channel classes as well
 - add overload taking both a properties filter and a RCCList (similar to the
current private utility function)
 - disallow the rccSubset key in the filter variantmap for the current version
(also prevents the only-detected-at-runtime "rccSubset isn't a set of rcc's"
filter invalidity case)

That probably leads to specifying and storing the filter and the rcc list in
the AccountSet object separately, which also would simplify the somewhat messy
special-casing currently present in the filter validity check and
accountMatchFilter() (should be accountMatchesFilter() in English, actually
:P). However if you decide to preserve them as-is, then just:
 - add RCCList AccountSet::capabilitiesFilter() in addition to the current
filter()
 - filter out rccSubset from the properties returned in the current filter().
However, I think it'd be clearer to go all the way, and add AccountSet
constructor separate param overloads similarly to the filterAccounts
suggestion.

A smaller thing is I think the naming of supportsTextChatsAccountsSet() and
friends is a bit awkward. Why not just supportsTextChatAccounts() (note the
singular form) or even just textChatAccounts() (mediaCallAccounts,
fileTransferAccounts, etc...)? We don't call ContactManager::allKnownContacts()
allKnownContactsSet() or AccountManager::accountsByProtocol()
accountsByProtocolSet() either. However, I figure the validAccountsSet(), ...
methods added earlier already have this unfortunate naming (to not collide with
the deprecated forms?) so we might have to be awkward for consistency. Let's
call them textChatAccountsSet() etc to not necessitate tp-qt4 users to set
tw+=100 again at least though? :P

On the test front, could we have a test stressing
AccountWrapper::checkCapabilitiesChanged()? Seems like a fragile piece of logic
with potential for regressions. Should be doable, having some additional
channel class in the test manager file and the CM, but not on the connected
connection's RCC. I realize currently the tests don't connect any accounts,
would this cause a dramatical increase in the test complexity? (Having in the
test one account with a real test CM which could be connected).

That's all I can think of now. Feel free to disagree on any of the points :)

-- 
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.