Bug 29090

Would it be more useful if this feature used Protocol objects when they get undrafted (Bug #20774), so we can return all online accounts that can actually make calls, plus all offline accounts that might be able to make calls?

(To have the necessary information to do that, you'd need to prepare a version of Tp::Account::FeatureProtocolInfo that made a Tp::Protocol object behind the scenes.)

If you don't want to wait for Protocol objects to be undrafted, please consider how you'd add them to the API later. Perhaps the API added by this bug could be named and documented to indicate that it only returns online accounts, and then you could later add a version of it that can also return offline accounts?

(In reply to comment #1)
> Would it be more useful if this feature used Protocol objects when they get
> undrafted (Bug #20774)

... oh, I see it does that. Never mind :-)

Ping

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 :)

Agreed with Andre on IRC that actually the best way might be to define a AccountFilter interface, with subclasses AccountPropertyFilter and AccountCapabilitiesFilter. This allows applications (and us in the future) to implement arbitrary additional filters, and is architecturally cleaner. For convenience, filterAccounts overloads taking property variant maps (constructing the equivalent AccountPropertyFilter) and RCCLists (constructs the CapsFilter) should be provided.

Thinking about it overnight, for future extensibility into filtering Contacts, I think we should after all make it template class Filter<typename T>, with one method virtual bool matches(SharedPtr<T>) (and obviously the virtual destructor). Then, once we have propertified Contact and added ConnectionCapabilities *Account::capabilities() const, which does the logic for using the capabilities transparently from the connected connection or the manager file, we can implement PropertyFilter<T> : public Filter<T>, GenericCapabilityFilter<T> : public Filter<T>, AccountCapabilityFilter : public GenericCapabilityFilter<Account> and ContactCapabilityFilter : public GenericCapabilityFilter<Contact>. This would allow analogous filtering of contacts by presence, supported channel types, etc quite neatly!

The KDE telepathy project would benefit from doing this contact filtering work as well - they're implementing filtering contact models on their own already.

More of a TODO list for myself, as I'm taking over this task:

AccountManager::FeatureFilterByCapabilities should make Account::FeatureCapabilities ready also on newly added accounts, not just the accounts initially found in introspection. As this means accounts getting added signal must be delayed until the feature is ready, I think this should only be finished after AccountFactory is added.

Most of the code in the filter classes doesn't really need to be template code. Code in PropertyFilter checking the properties should be refactored to a non-template function taking a QObject *, while code checking the RCC subset in the caps filter should be refactored into a non-template function taking the RCCList. This is beneficial both for compile times and the possibility to fix bugs by an updated shared library containing the implementations (with no need to recompile against newer tp-qt4).

There should be typedefs for the templated stuff.

The variant map version shouldn't be deprecated, but should be preserved as a shortcut constructing the equivalent propertyFilter. Similar shortcut should be added for a generic RCC filter. The additional filter channel class filter functions reverting should be reverted (:D).

Tests should be more exhaustive.

Typos: textChatsAccounts should be textChatAccounts, etc.

Account caps changing should only be emitted if there's an actual difference. Also, notify() should always be done.

Made this depend on #29606 so that the flaky FeatureFilterByCapabilities "proxy feature" can be instead implemented as "specify FeatureCapabilities in the Account factory if you want to filter by capabilities".

Merged to master. Will be in 0.3.10.