Bug 29672

The Utils functions are implemented as hand-written indexing based string algorithms, which makes for quickly executing code. As far as I can see, they're currently implemented correctly, but we both know such algorithms are easy to mess up if they need to be modified in any way (say, add new syntax support) - therefore, please add tests for them. The tests should be easy to write as there's no event-driven programming involved this time.

Tbh I think "Tp::variantFromValueWithDBusSignature" is a bit on the long side. How about parseValueWithSignature(), variantTypeForSignature() (you don't have to be consistent in naming with the API it deprecates in CM/ManagerFile/KeyFile)!

ProfileManager is exactly the API I was hoping for to enumerate profiles, good job ++.

I'll add some more after a quick lunch...

I think we should have another constructor Profile() which would be used in the case setFileName() will be called anyway to prevent searching for / parsing the wrong file completely. Then we should have createForServiceName() and createForFilename() wrapping it, instead of the current create(serviceName) so that it's clearer what will actually be done. I think it's sensible anyway to have the filename version be public too anyway.

You should either comment out the account profile stuff OR implement the fake profile, otherwise it's terribly confusing as mostly anybody using tp-qt4 on desktop will find that their account doesn't have a profile in fact.
 - Agreed on IRC that fake profiles should be implemented, and that Account profiles should be Account::FeatureProfile, which depends on Account::FeatureProtocolInfo. This is needed to get the default parameters for the protocol to specify as optional service-specific parameters in the fake profile, but additionally allows us to potentially verify that the Protocol parameters / RCCs / etc as reported by the CM or its .manager file are actually compatible with info in an actual .profile file.

In startElement, I don't think you should error out on unrecognized elements. If we happen to add new elements in a .profile file format update, we don't want old tp-qt4 versions to ignore them. Instead, in case you don't recognize the element, just leave out the check for not having attrs and being a child of the service node. Emit a warning about ignoring the element and continue parsing normally.

The API leaves answering the question "for this set of fixedProperties in unsupportedChannelClasses, which capabilities are actually still available" quite hard, so UCC is quite useless :(. Do you think it would be sensible in the filter-by-rcc branch to add to the Account profile support something, that when there is no Connected Connection and the ProtocolInfo is used, would auto-subtract the profile UCC from the ProtocolInfo RCC so that the Account Capabilities would then report supportTextChats() etc correctly? Note that when there is a Connected Connection, it will anyway not report the RCCs the service doesn't support - the CM is expected to do the subtraction.

The other option is to provide some kind of CapabilitiesBase proxy which takes a CapabilitiesBase and the unsupportedChannelClasses, and then acts based on the supplied CapabilitiesBase->requestableChannelClasses() and the unsupportedChannelClasses. Profile could have a function to return such a caps proxy instance. This seems a bit hairy though...

Do you see some other straightforward option?

If this capabilities badgering is too hard to do now, let's leave it for bug #29484 but in this case let's leave a big fat TODO. We need to do something about RCCs in Profiles though to support eg. account creation UIs wanting to show "this profile would support voice calls, choose it!" to the user creating an account - there's no Account at that point, just the Profile exposing bare RCCs.

allowOthersPresences should be allowOtherPresences.

Otherwise ++, I think. Please drop a comment on this bug when these are settled!

(In reply to comment #1)
> The Utils functions are implemented as hand-written indexing based string
> algorithms, which makes for quickly executing code. As far as I can see,
> they're currently implemented correctly, but we both know such algorithms are
> easy to mess up if they need to be modified in any way (say, add new syntax
> support) - therefore, please add tests for them. The tests should be easy to
> write as there's no event-driven programming involved this time.
As said on IRC, these methods are already well tested in tests/key-file.cpp and tests/manager-file.cpp, so no change here.

> Tbh I think "Tp::variantFromValueWithDBusSignature" is a bit on the long side.
> How about parseValueWithSignature(), variantTypeForSignature() (you don't have
> to be consistent in naming with the API it deprecates in
> CM/ManagerFile/KeyFile)!
Done, renamed as suggested.

> ProfileManager is exactly the API I was hoping for to enumerate profiles, good
> job ++.
:)

(In reply to comment #2)
> I think we should have another constructor Profile() which would be used in the
> case setFileName() will be called anyway to prevent searching for / parsing the
> wrong file completely. Then we should have createForServiceName() and
> createForFilename() wrapping it, instead of the current create(serviceName) so
> that it's clearer what will actually be done. I think it's sensible anyway to
> have the filename version be public too anyway.
Implemented this but a little different than suggested. Profile now has only one constructor that takes no parameter and private methods setServiceName and setFileName that resets the profile data and re-parse it.
We can't have constructors taking protocol name and service name, as both are QString...

Also renamed create to createForServiceName and added a new public method createForFileName as suggested.

> You should either comment out the account profile stuff OR implement the fake
> profile, otherwise it's terribly confusing as mostly anybody using tp-qt4 on
> desktop will find that their account doesn't have a profile in fact.
>  - Agreed on IRC that fake profiles should be implemented, and that Account
> profiles should be Account::FeatureProfile, which depends on
> Account::FeatureProtocolInfo. This is needed to get the default parameters for
> the protocol to specify as optional service-specific parameters in the fake
> profile, but additionally allows us to potentially verify that the Protocol
> parameters / RCCs / etc as reported by the CM or its .manager file are actually
> compatible with info in an actual .profile file.
Done.

> In startElement, I don't think you should error out on unrecognized elements.
> If we happen to add new elements in a .profile file format update, we don't
> want old tp-qt4 versions to ignore them. Instead, in case you don't recognize
> the element, just leave out the check for not having attrs and being a child of
> the service node. Emit a warning about ignoring the element and continue
> parsing normally.
I think this can be done by changing xmlns with a new profile definition. Changed anyway, it only warns now.

> The API leaves answering the question "for this set of fixedProperties in
> unsupportedChannelClasses, which capabilities are actually still available"
> quite hard, so UCC is quite useless :(. Do you think it would be sensible in
> the filter-by-rcc branch to add to the Account profile support something, that
> when there is no Connected Connection and the ProtocolInfo is used, would
> auto-subtract the profile UCC from the ProtocolInfo RCC so that the Account
> Capabilities would then report supportTextChats() etc correctly? Note that when
> there is a Connected Connection, it will anyway not report the RCCs the service
> doesn't support - the CM is expected to do the subtraction.
I think Profile::unsupportedChannelClasses should not be used directly by UIs, instead they should use Account::capabilities that will be a merge of CM caps and profile caps (or just the connection caps if online). The branch filter-by-rcc adds a method capabilities() that should do the trick when creating a ConnectionCapabilities object.

I can't see any better option here, we could add a ConnectionCapabilities *unsupportedCapabilities() method instead, but it will be confusing anyway.

> The other option is to provide some kind of CapabilitiesBase proxy which takes
> a CapabilitiesBase and the unsupportedChannelClasses, and then acts based on
> the supplied CapabilitiesBase->requestableChannelClasses() and the
> unsupportedChannelClasses. Profile could have a function to return such a caps
> proxy instance. This seems a bit hairy though...
>
> Do you see some other straightforward option?
As said above I think this is work for Account::capabilities added in filter-by-rcc. Now whether to return a RCCList or a ConnectionCapabilities * in Profile::unsupportedCC/Caps I don't know.
 
> If this capabilities badgering is too hard to do now, let's leave it for bug
> #29484 but in this case let's leave a big fat TODO. We need to do something
> about RCCs in Profiles though to support eg. account creation UIs wanting to
> show "this profile would support voice calls, choose it!" to the user creating
> an account - there's no Account at that point, just the Profile exposing bare
> RCCs.
Once we decide about this I can update the bug. IMHO we should just leave as is and make Account::capabilities do the work for us.

> allowOthersPresences should be allowOtherPresences.
Done :)

> Otherwise ++, I think. Please drop a comment on this bug when these are
> settled!
Thanks for the review, pushed a new version with the fixes stated and also added more tests for Account::profile and docs.

I'll make the CM protocol branch make the fake protocol's icon name whatever the CM protocol indicates instead of always having im-protocol, I think that's sensible?

Given that we say "+ * In general, services of interest of telepathy should be of type 'IM'.
+ * Other service types exist but are unlikely to affect telepathy in any way.", should we actually ignore other types in the lists returned by ProfileManager (ie. continue the search in the search path until we find one that is both valid AND has type == "IM", omitting the service if there's none). The same for Profile::createForServiceName(), but Profile::createForFilename() should still obviously return a Profile even if it has some other type. Otherwise we end up with account creation UIs showing non-IM profiles in dialogs for creating IM accounts...

If you start ignoring the != "IM" accounts, emit debug for each filename thus ignored.

Or do we have some other use case than "in general"?

Add a one test case which uses Profile::createByFileName() please. And if you ignore non-IM stuff, add one non-IM profile earlier in the search path for some service you currently have a test for (with some discerning attribute you can check for) and check that the later one is used.

If the spec file format is sufficiently stable to do so, merge away after doing the above trivial adjustments. I'll include this in the next release then.

(In reply to comment #5)
> If you start ignoring the != "IM" accounts, emit debug for each filename thus
> ignored.
> 

!= "IM" *profiles*, rather

Too many concepts :)

Review of the latest state of the branch:

The latest format support patch makes having a provider and an icon mandatory. The other elements-turned-attributes are crucial, but I don't think the provider and - more so - the icon should be mandatory! Well, as the spec still hasn't been totally agreed on, I guess they could be made mandatory there as well at any point, but I think we still should fail gracefully in that case. Making the icon optional probably means that the Account iconName() fallback for the Account not having an icon set should only use the profile icon if there is one, otherwise the protocol icon, even if there is a real profile. (And that this behavior should be documented). Otherwise, this patch is OK.

PendingOperation dontFailOnFirstError patch: ++

Fake profile support: Woo, this actually makes listNames() finally tested by the unit tests :) However, you could elaborate a bit more on what "fake profile" and "all protocols" actually means in the feature doc comment - the latter could be something like "all protocols supported on the installed connection managers, even if they don't have profile files installed making use of them", I think. ALSO (more critical): shouldn't you check which CMs are, in fact, ready in the onCMsReady() loop (if one of them failed for some reason?).

Account::profile()::serviceName() for fake profiles change: Hrm, this is strictly speaking backwards incompatible, but I don't think anybody depends on the naming of the *fake* profiles yet. So ++.

Moving utility functions around: ++

++ for merge, but address these first (ask for clarification/open discussion if needed).

The latest changes in this branch were very cleanly organized, thanks for that!

Bah, of course we have to wait for the format to settle still before the merge!

(In reply to comment #7)
> Review of the latest state of the branch:
> 
> The latest format support patch makes having a provider and an icon mandatory.
> The other elements-turned-attributes are crucial, but I don't think the
> provider and - more so - the icon should be mandatory! Well, as the spec still
> hasn't been totally agreed on, I guess they could be made mandatory there as
> well at any point, but I think we still should fail gracefully in that case.
> Making the icon optional probably means that the Account iconName() fallback
> for the Account not having an icon set should only use the profile icon if
> there is one, otherwise the protocol icon, even if there is a real profile.
> (And that this behavior should be documented). Otherwise, this patch is OK.
Changed icon and provider to be optional and changed Account::iconName to use Profile::iconName in favor of ProtocolInfo::iconName if set. Updated Account::iconName docs accordingly.

> Fake profile support: Woo, this actually makes listNames() finally tested by
> the unit tests :) 
:)

> However, you could elaborate a bit more on what "fake
> profile" and "all protocols" actually means in the feature doc comment - the
> latter could be something like "all protocols supported on the installed
> connection managers, even if they don't have profile files installed making use
> of them", I think. 
Fixed.

> ALSO (more critical): shouldn't you check which CMs are, in
> fact, ready in the onCMsReady() loop (if one of them failed for some reason?).
Fixed.

> Account::profile()::serviceName() for fake profiles change: Hrm, this is
> strictly speaking backwards incompatible, but I don't think anybody depends on
> the naming of the *fake* profiles yet. So ++.
Actually there is no problem here as there was no profile support merged yet, this is all new.
 
> The latest changes in this branch were very cleanly organized, thanks for that!
Thanks :)

Added some other improvements also, as emit change notifications for iconName and profile (if needed) when the service name changes (setServiceName).

Merged upstream. It will be in next version 0.3.11.

And closing the bug :)