Bug 37034

(In reply to comment #0)
> Telepathy-Qt4 doesn't set the URI property for file transfers.
> Therefore if the handler is not the same that initiates the transfer channel,
> it is impossible to know the location of the file that should be sent and to
> open the file in order to call
> OutgoingFileTransferChannel::provideFile(QIODevice* input).    
> 
> FileTransferChannelCreationProperties only accepts a "suggestedFileName".
> Moreover accounts.cpp:271 uses QFileInfo for a suggestedFileName. If the file
> name is just "suggested" it could be set by the user and could not correspond
> to a real file. (This is probably another bug)
This is not a bug, it just binds http://telepathy.freedesktop.org/spec/Channel_Type_File_Transfer.html#Property:Filename which states this is a suggested filename.

> Therefore if the "suggestedFileName" corresponds to a real file, it is possible
> to set the URI property getting the absolute path, otherwise it is impossible.
> 
> I can work on this but I need to know how should I fix it...
> 
> 1) Add a setUri method to FileTransferChannelCreationProperties
> 2) Add a constructor to FileTransferChannelCreationProperties(const QString
> &uri) (that might also set size, hash, content, etc)
> 3) Just rename "suggestedFileName" to "uri", fix documentation and hope that
> nobody uses it yet with a non existent file.
> 
> In my opinion #2 is the best.
> 
> I also suggest that (but this will break abi compatibility)
> - the "FileTransferChannelCreationProperties(const QString &suggestedFileName,
>   const QString &contentType, qulonglong size);" constructor should be removed;
> - a method setSuggestedFileName(const QString &suggestedFileName) should be
>   added to override the default name extracted from the URI.

What I suggest is to mix 1 and 2, but the constructor should take the uri and suggestedFileName and have a setSuggestedFileName as well. If the suggestedFilename passed is empty use uri. setSuggestedFilename should do the same if the filename is empty and use uri instead.
 
And please do not remove anything, just deprecate the other constructor taking only a suggested filename. Adding new methods won't break API.

Also add a FT::uri() accessor.

> > FileTransferChannelCreationProperties only accepts a "suggestedFileName".
> > Moreover accounts.cpp:271 uses QFileInfo for a suggestedFileName. If the file
> > name is just "suggested" it could be set by the user and could not correspond
> > to a real file. (This is probably another bug)
> This is not a bug, it just binds
> http://telepathy.freedesktop.org/spec/Channel_Type_File_Transfer.html#Property:Filename
> which states this is a suggested filename.

In my opinion the bug is the use of QFileInfo...

Anyway I made some changes and uploaded here [1] a first version of the patch for review... bonus feature: last commit adds documentation to the FileTransferChannelCreationProperties class (referring to the version modified by me, not to the original one)

[1]https://www.gitorious.org/drdanz-telepathy-kde/telepathy-qt4/commits/filetransfer-uri

(In reply to comment #2)
> In my opinion the bug is the use of QFileInfo...
> 

AFAIK, a filename passed to QFileInfo doesn't have to refer to an existant file (there is an exists() accessor). The use of QFileInfo is intended to weed out any path components in case somebody passes those to us, so in essence it's used just as "basename". Thus, I would continue using it, unless you've discovered some actual problem with it.

> Anyway I made some changes and uploaded here [1] a first version of the patch
> for review... bonus feature: last commit adds documentation to the
> FileTransferChannelCreationProperties class (referring to the version modified
> by me, not to the original one)
> 
> [1]https://www.gitorious.org/drdanz-telepathy-kde/telepathy-qt4/commits/filetransfer-uri


    bool hasUri;
63	    QString uri;

I don't think an empty URI makes sense. So I don't think you need the separate hasUri variable.

Your added setters for the mandatory params have this:

   if (!isValid()) {
114	        // there is no point in updating content hash if not valid, as we miss filename, content
115	        // type and size
116	        return *this;
117	    }

Aside from the out of place comment, wouldn't this mean you can't actually use these setters to make an invalid object valid? That's actually fine, though. If you're changing the mandatory params definining the file transfer, you probably need to re-set the optional ones too. This was accomplished by the old API forcing you to create a new instance (which is not a problem as the class is very lightweight). What's the rationale for 

Another misplaced comment in setContentType:

	135	    if (contentType.isEmpty()) {
136	        // suggestedFileName is a mandatory parameter, therefore we cannot set an empty string
137	        return *this;
138	    }

Besides, please do warn in such occasions which you make a no-op based on arbitrary criteria like this.

> Use isValid() method to check if mandatory properties are set

This commit makes all operations on default-constructed CreationProperties to instantly SIGSEGV, including isValid(), with no way to detect that from the outside. Additionally, this change would make the class wildly inconsistent with our other tiny value-semantic classes, for which isValid() means "this is not the null object, as used as a placeholder when there is no actual value". What's the point in this change? You can still ditch the object you were constructing and return a null object if something goes wrong e.g. in resolving the URI to a file even with the old, consistent approach.

> Add constructor to FileTransferChannelCreationProperties accepting uri and description

Can't you use QUrl to parse the URI? (And validate it's one in the first place, otherwise warn and return an invalid instance). That class even has a toLocalFile() accessor which would be useful.

Also, as description is an optional parameter, please stick to the named parameter idiom and instead of having it as a create() param, have it set by a setter method, which leads to more readable code. If this would lead to some overload conflict with previously existing create() methods, please call the new one createForURI or somesuch. You could actually use QUrl as an (overload) parameter type directly.

In short, please redo the branch, this time focusing on what features you actually want to add (URI passing), doing that well, and not arbitrarily changing the structure and semantics of the class otherwise. For history readability, this is best done by rebasing the commit I actually liked (adding documentation) on top of master, ditching most of the others while stashing the good bits, and starting again from there.

(In reply to comment #0)
> Telepathy-Qt4 doesn't set the URI property for file transfers.
> Therefore if the handler is not the same that initiates the transfer channel,
> it is impossible to know the location of the file that should be sent and to
> open the file in order to call
> OutgoingFileTransferChannel::provideFile(QIODevice* input).    

Very much true, so indeed we should add support for the new URI property - but we also need a getter for it in FileTransferChannel, for the Handler to actually get to the information with a nice API.

We also need change notification and a setter for the property in IncomingFileTransferChannel for the usecase mentioned in the spec:

For incoming file transfers, this property MAY be set by the channel handler before calling AcceptFile to inform observers where the incoming file will be saved.

> 
> FileTransferChannelCreationProperties only accepts a "suggestedFileName".
> Moreover accounts.cpp:271 uses QFileInfo for a suggestedFileName. If the file
> name is just "suggested" it could be set by the user and could not correspond
> to a real file. (This is probably another bug)
> 
> Therefore if the "suggestedFileName" corresponds to a real file, it is possible
> to set the URI property getting the absolute path, otherwise it is impossible.

Please read the spec, as Andre linked. The suggested filename is a distinct concept from the URI.

> 
> I can work on this but I need to know how should I fix it...
> 
> 1) Add a setUri method to FileTransferChannelCreationProperties

No. I think this would be useless if we have 2).

> 2) Add a constructor to FileTransferChannelCreationProperties(const QString
> &uri) (that might also set size, hash, content, etc)

Yes.

> 3) Just rename "suggestedFileName" to "uri", fix documentation and hope that
> nobody uses it yet with a non existent file.

No. The suggested filename is still a valid concept, and should be able to be set independently of the URI filename if desired (once an instance is built by the URI create method(), to override the filename extracted thus).

> 
> In my opinion #2 is the best.

Agreed.

(In reply to comment #3)
> (In reply to comment #2)
> > In my opinion the bug is the use of QFileInfo...
> > 
> 
> AFAIK, a filename passed to QFileInfo doesn't have to refer to an existant file
> (there is an exists() accessor). The use of QFileInfo is intended to weed out
> any path components in case somebody passes those to us, so in essence it's
> used just as "basename". Thus, I would continue using it, unless you've
> discovered some actual problem with it.

I thought that the string had to correspond to a real file, but I made some tests and I was wrong, so I'll revert this. The only problem I found with it is that if the string ends with "/", fileName is empty, but I guess that's actually not a real problem.


> Your added setters for the mandatory params have this:
> 
>    if (!isValid()) {
> 114            // there is no point in updating content hash if not valid, as
> we miss filename, content
> 115            // type and size
> 116            return *this;
> 117        }
> 
> Aside from the out of place comment, wouldn't this mean you can't actually use
> these setters to make an invalid object valid?

This is in the 3rd commit, where isValid() is not modified yet.
isValid() returns false only if mPriv was not created, that means that the empty constructor was used.
The comment is in that place for coherence with the other setters in the same file...

In the 5th commit the behaviour of isValid is modified to return true if mandatory parameters are set, the default constructor is modified so that constructs an empty mPriv and all the "isValid" checks in setters (and also in getters) are removed, in order to actually use these setters to make the object valid.

It was probably better to invert the order of the commits, but the first 4 commits were supposed to be "light" commits that don't modify the behaviour of the class and I wasn't really convinced by the 5th commit...

> > Use isValid() method to check if mandatory properties are set
> 
> This commit makes all operations on default-constructed CreationProperties to
> instantly SIGSEGV, including isValid(), with no way to detect that from the
> outside. Additionally, this change would make the class wildly inconsistent
> with our other tiny value-semantic classes, for which isValid() means "this is
> not the null object, as used as a placeholder when there is no actual value".
> What's the point in this change? You can still ditch the object you were
> constructing and return a null object if something goes wrong e.g. in resolving
> the URI to a file even with the old, consistent approach.

This commit also modifies the behaviour of the default constructor, to construct an empty mPriv, and you can use it and set mandatory parameters later.
Therefore it shouldn't SIGSEGV
Actually I thought that the meaning of "isValid()" was "This is something that you can use to create a valid channel request.

I believe that some check like this is needed, because creating the CreationProperties by URI won't set the mandatory parameters if the file is not a local file (Specifications explicitly affirm that "For outgoing files, this URI MAY use a different scheme, such as http:, if a remote resource is being transferred to a contact."

Suggestions?



> If you're changing the mandatory params definining the file transfer, you
> probably need to re-set the optional ones too.

Not necessarily, see previous comment and the documentation (6th commit) for default constructor and constructor by uri


> Another misplaced comment in setContentType:
> 
>     135        if (contentType.isEmpty()) {
> 136            // suggestedFileName is a mandatory parameter, therefore we
> cannot set an empty string
> 137            return *this;
> 138        }

Where should the comment be? I just copied the style of the comments in the other methods...



> Also, as description is an optional parameter, please stick to the named
> parameter idiom and instead of having it as a create() param, have it set by a
> setter method, which leads to more readable code. If this would lead to some
> overload conflict with previously existing create() methods, please call the
> new one createForURI or somesuch. You could actually use QUrl as an (overload)
> parameter type directly.

That was supposed to be a constructor that can be used (if the file is a local file) to set at the same time all the properties. Most properties can be set from the file, the only one that cannot be read is the description, therefore I added it, as it didn't see any reason not to do it...
Anyway I'll remove it.


> In short, please redo the branch, this time focusing on what features you
> actually want to add (URI passing), doing that well, and not arbitrarily
> changing the structure and semantics of the class otherwise.

URI passing is actually just the 2nd commit...
But in order to add a constructor that accepts URI I believe that some changes are required, otherwise it will have problems with remote files.

Anyway in next I will also add the getter in FileTransferChannel and the setter in IncomingFileTransferChannel...

Should I add the signal URIDefined as well or is it done automatically? In FileTransferChannel or in IncomingFileTransferChannel? (In the specs it's in FileTransferChannel, but I guess it makes no sense if the setter is only in IncomingFileTransferChannel...)

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > In my opinion the bug is the use of QFileInfo...
> > > 
> > 
> > AFAIK, a filename passed to QFileInfo doesn't have to refer to an existant file
> > (there is an exists() accessor). The use of QFileInfo is intended to weed out
> > any path components in case somebody passes those to us, so in essence it's
> > used just as "basename". Thus, I would continue using it, unless you've
> > discovered some actual problem with it.
> 
> I thought that the string had to correspond to a real file, but I made some
> tests and I was wrong, so I'll revert this. The only problem I found with it is
> that if the string ends with "/", fileName is empty, but I guess that's
> actually not a real problem.
> 

In this case the path doesn't refer to a file we could send anyway, so the best thing we can do is to warn and return the null object if such a path is passed in.

> 
> > Your added setters for the mandatory params have this:
> > 
> >    if (!isValid()) {
> > 114            // there is no point in updating content hash if not valid, as
> > we miss filename, content
> > 115            // type and size
> > 116            return *this;
> > 117        }
> > 
> > Aside from the out of place comment, wouldn't this mean you can't actually use
> > these setters to make an invalid object valid?
> 
> This is in the 3rd commit, where isValid() is not modified yet.
> isValid() returns false only if mPriv was not created, that means that the
> empty constructor was used.
> The comment is in that place for coherence with the other setters in the same
> file...

I meant that all comments say "updating content hash" and that set of missing things even in setters other than the content hash one.

> 
> In the 5th commit the behaviour of isValid is modified to return true if
> mandatory parameters are set, the default constructor is modified so that
> constructs an empty mPriv and all the "isValid" checks in setters (and also in
> getters) are removed, in order to actually use these setters to make the object
> valid.

mPriv is a pointer. Your modification to the default constructor's initial list is a no-op: you're default-constructing the pointer as before, though you've now written that explicitly. The default value for pointers is NULL, which triggers a crash in all accessors the way you modified them.

Again, it would be conceptually a lot cleaner if you had the null object (as created by the default constructor, and others if invalid args are passed), and no way to try and mend the object to make it valid. This is the semantics for all other similar classes in TpQt4 as well in addition to being the existing behavior for this specific class.

> 
> It was probably better to invert the order of the commits, but the first 4
> commits were supposed to be "light" commits that don't modify the behaviour of
> the class and I wasn't really convinced by the 5th commit...
> 
> > > Use isValid() method to check if mandatory properties are set
> > 
> > This commit makes all operations on default-constructed CreationProperties to
> > instantly SIGSEGV, including isValid(), with no way to detect that from the
> > outside. Additionally, this change would make the class wildly inconsistent
> > with our other tiny value-semantic classes, for which isValid() means "this is
> > not the null object, as used as a placeholder when there is no actual value".
> > What's the point in this change? You can still ditch the object you were
> > constructing and return a null object if something goes wrong e.g. in resolving
> > the URI to a file even with the old, consistent approach.
> 
> This commit also modifies the behaviour of the default constructor, to
> construct an empty mPriv, and you can use it and set mandatory parameters
> later.
> Therefore it shouldn't SIGSEGV

It does, see above

> Actually I thought that the meaning of "isValid()" was "This is something that
> you can use to create a valid channel request.

Yes, it's not that. Sorry for the missing documentation. The other way around is true, though: !isValid() means that "passing this in for a request makes no sense at all, because it's the null object".

> 
> I believe that some check like this is needed, because creating the
> CreationProperties by URI won't set the mandatory parameters if the file is not
> a local file (Specifications explicitly affirm that "For outgoing files, this
> URI MAY use a different scheme, such as http:, if a remote resource is being
> transferred to a contact."
> 
> Suggestions?
> 
> 

Then you need to have an another create which accepts non- file: URLs and the required supplementary parameters, and warn and return the default-constructed object in the just-an-URL one for non-local URLs.

> 
> > If you're changing the mandatory params definining the file transfer, you
> > probably need to re-set the optional ones too.
> 
> Not necessarily, see previous comment and the documentation (6th commit) for
> default constructor and constructor by uri

Not necessarily, but sometimes/often you do. Allowing potentially conflicting changes to the mandatory parameters after creation doesn't bring any advantage, just confusion possibilities, as long as you have enough creation methods to cover all cases (such as apparently local and remote URLs separately).

> 
> 
> > Another misplaced comment in setContentType:
> > 
> >     135        if (contentType.isEmpty()) {
> > 136            // suggestedFileName is a mandatory parameter, therefore we
> > cannot set an empty string
> > 137            return *this;
> > 138        }
> 
> Where should the comment be? I just copied the style of the comments in the
> other methods...
> 

The style might be copied, but not the contents verbatim. This comment is from the content type setter, yet it refers to the suggested file name.

> 
> 
> > Also, as description is an optional parameter, please stick to the named
> > parameter idiom and instead of having it as a create() param, have it set by a
> > setter method, which leads to more readable code. If this would lead to some
> > overload conflict with previously existing create() methods, please call the
> > new one createForURI or somesuch. You could actually use QUrl as an (overload)
> > parameter type directly.
> 
> That was supposed to be a constructor that can be used (if the file is a local
> file) to set at the same time all the properties. Most properties can be set
> from the file, the only one that cannot be read is the description, therefore I
> added it, as it didn't see any reason not to do it...
> Anyway I'll remove it.
> 

Thanks. A good reason to not include it is consistency with the other create methods, which don't have a way to pass in any of the optional parameters (or description specifically) either.

> 
> > In short, please redo the branch, this time focusing on what features you
> > actually want to add (URI passing), doing that well, and not arbitrarily
> > changing the structure and semantics of the class otherwise.
> 
> URI passing is actually just the 2nd commit...
> But in order to add a constructor that accepts URI I believe that some changes
> are required, otherwise it will have problems with remote files.

Rather add two constructors as suggested above, please.

A general guideline: You can only add new API. You can't change the behavior and/or semantics of existing API. Other than breaking strict backwards compatibility, doing so would make the class inconsistent with the rest of TpQt4.

> 
> Anyway in next I will also add the getter in FileTransferChannel and the setter
> in IncomingFileTransferChannel...

Great!

> 
> Should I add the signal URIDefined as well or is it done automatically? In
> FileTransferChannel or in IncomingFileTransferChannel? (In the specs it's in
> FileTransferChannel, but I guess it makes no sense if the setter is only in
> IncomingFileTransferChannel...)

Ah, true, there is no general change notification for the property, just notification for it being defined in the first place. The signal is not in any way automatically added to the high-level class: the autogeneration is limited to the low-level Tp::FileTransferChannelInterface class. Thus you need to add a new signal to IncomingFileTransferChannel and connect the FTCInterface generated signal to it. You don't actually need a general-purpose change notification signal in that case.

We should place the signal in IFTC only, because the spec explicitly states that the property must remain fixed for the lifetime of an outgoing file transfer, so we don't need the signal or the setter there.

I just pushed the new version of the branch here [1]

[1]https://www.gitorious.org/drdanz-telepathy-kde/telepathy-qt4/commits/filetransfer-uri

Reviewing again.

> Clean suggestedFileName when it is stored instead of when it is used in FileTransferChannelCreationProperties

Good, this makes sense!

244 request.insert(QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER ".URI"),
245 properties.uri());

This will unconditionally (even if !hasURI) insert the URI key to the request. This is wrong; it'd make all existing users of the API pass the URI key with an empty value (which is invalid) in the request.

On that subject, where actually is the hasURI method mentioned in the commit message? Otherwise the commit adding the URI accessors looks fine.

> Add constructor that accepts URI property to
> FileTransferChannelCreationProperties

No! This is again inconsistent with the other optional properties. *Only the mandatory properties should be set by the constructor, the others by the setters following the named parameter idiom*. When we're passing the mandatory properties (suggested fn, content type, size) explicitly, the URI is *not* a mandatory property. Hence I don't see any reason this constructor should be there.

Revert, please.

> Set URI property if suggestedFileName is a local file when creating 
> FileTransferChannelCreationProperties

NO! Again, you're changing behavior of the (only) previously existing API here. Previously, the intended usage of the API was to pass the suggested file basename (not a path). The use of QFileInfo to strip out any path components was only a robustness measure (Andre, as the original writer of this code, if you're reading this, correct me if I'm wrong). Thus you've now made the previously intended usage emit a warning, or in some cases (if the file is actually in our working directory), additionally set a new request property.

Revert, please.

: contentType(QLatin1String("application/octet-stream")), //TODO set contentType from content of the file

I don't think you can in the general case, without reimplementing file(1) and then some. Hence, you must retain the content type as a separate parameter.

            // TODO set contentHash and contentHashType in this method

You can't, because the content hash type doesn't depend on the file: it depends on the content hash types supported by the protocol / connection manager in question. Multiple ones might be applicable. Thus, you'd need a separate constructor parameter to choose the desired content hash type, and as the content hash is optional that'd again be inconsistent with the other optional parameters.

Otherwise a constructor which takes a path, or an URL to a local file (whichever seems more convenient to you from an application POV) is a good idea.

92	        } else {
93	            warning() << path << "is not a local file.";
94	        }

As the required properties (at least size) couldn't be set in this case, you must arrange for the constructor to return an !isValid() object. My suggestion: check in this public constructor for mPriv->suggestedFileName being empty, in which case make mPriv NULL.

Since you're online I'm posting this part now so you can get started. I'll review the FTChannel additions in a separate comment and the docs afterwards (you'll probably adjust them a bit for these changes, right?).

Oh and please add an unit test, which checks the operation of the path constructor both when passed an existent file and a nonexistent one as this is non-trivial. (First case: valid one, with the size of some small actual file you added. Second case: invalid.)

This test can be one case of the Account test which tests a lot of other Account channel requesting things (TestAccountChannelDispatcher IIRC), you don't have to create a separate test application.

98	    parent->connect(fileTransferInterface,
99	            SIGNAL(URIDefined(QString)),
100	            SLOT(onUriDefined(QString)));

Umm, why don't I see this slot anywhere?

Remember that this can only change for Incoming FTs, so you only need to connect to it in IFTC, not here at all. I suppose you need to connect the IFTC signal to onUriChanged, though, to update the URI property returned by the accessor? You might want to do that in the subclass constructor (because the signal exists only for that subclass).

153   warning() << "FileTransferChannel::FeatureCore must be ready before "
154      "calling acceptFile";

Yet the user has called setUri, not acceptFile. Otherwise a warning is a good idea in this case.

322	 * is requested. For incoming file transfers, this property may be set by the
323	 * channel handler before calling AcceptFile to inform observers where the
324	 * incoming file will be saved.

Please mention the IFTC signal which will be emitted when this happens.

In the docs, obviously the things pertaining to the actual code/API changes required need changing, other than that they look good, except for a few nitpicks:

199	/**
200	 * Create a FileTransferChannelCreationProperties used to initiate the
201	 * properties for a file transfer channel.
202	 */

/** Copy constructor. */ please, as this is nothing more.

> ... used to initiate the
> 133	 * properties for a file transfer channel.
> ... will be used when requesting a new file transfer
> 231	 * channel
> will be used when requesting a new file transfers
> 277	 * channel.

You don't need to repeat the class's purpose in every method's description. Describe these kinds of things in the class description, and only describe what a given method does in the class context in the method ones (e.g. "Sets a content hash for the request").

Also in the accessors, please refer to concepts ("content hash") rather than D-Bus properties ("the ContentHash and ContentHashType properties").

Updated.
Sorry I had no time to write the unit tests today...

Hello,

I am on a training until Tuesday, 31.5. I will be back on 1.6. I will check my
emails with delay, so for very urgent matters,
call/SMS me (+358 50 482 77 86).

Simone (simone.leggio@nokia.com, +358 50 486 9494 is my deputy.

Best Regards,
-Baris

Reeview,

> Do not use bool variables to remember if FileTransferChannelCreationProperties > has properties

++

> Support the URI property in FileTransferChannelCreationProperties

Looks good now!

> Add constructor that sets properties from path to a local file to
> FileTransferChannelCreationProperties

Ditto!

> Add documentation for class FileTransferChannelCreationProperties

The docs look spot on now.

	98	    parent->connect(fileTransferInterface,
99	            SIGNAL(URIDefined(QString)),
100	            SLOT(onUriDefined(QString)));

There still is this connection to a non-existent slot (unless you are actually a IncomingFileTransferChannel) in the FileTransferChannel baseclass, and the onUriChanged slot which is not used anywhere. Please re-read my previous comment on the subject, adjusting this to only connect to the D-Bus signal in IFTC, and use some backdoor (such as a protected slot) in the parent object to set the parent's property to the value defined.

Actually it seems you missed my comments in comment 9 in general.

Waiting for the comment 9 changes and the unit tests.

Yeah sorry, I definitely missed #c9
Updated and added some unit tests for FileTransferChannelCreationProperties and for Account::createFileTransfer() method,
I'm not sure if the last one is what I was supposed to test, anyway the tests for that method are missing

A problem I noticed is that the channel is requested even if the FileTransferChannelCreationProperties is not valid (not only by using the path constructor, but also using the default constructor). Maybe an error should be returned before requesting the channel, but the method returns a PendingChannelRequest* and therefore it is not possible to return a PendingFailure*, maybe a PendingChannelRequestFailure class should be implemented? Or should it be left as it is now and just start the request waiting for the failure from CM?

> A problem I noticed is that the channel is requested even if the
> FileTransferChannelCreationProperties is not valid (not only by using the path
> constructor, but also using the default constructor). Maybe an error should be
> returned before requesting the channel, but the method returns a
> PendingChannelRequest* and therefore it is not possible to return a
> PendingFailure*, maybe a PendingChannelRequestFailure class should be
> implemented? Or should it be left as it is now and just start the request
> waiting for the failure from CM?

No, we don't need a separate subclass for that. Just add a new ctor to the current one which takes the error name and message, and immediately calls setFinishedWithError without doing anything else. This is in fact how it's done with e.g. PendingHandles.

Subclassing wouldn't work because the current PCR ctor does a lot of things to start the channel request - so as any subclass would have to invoke it, it'd do those things as well.

Oh, you went the distance of adding a separate testcase! I'd say split the FTC test to 4 individual slots/testcases (corresponding to each of your current basic blocks in the one big slot). More readable, and gives more accurate output in the case of a test failure. Otherwise the test looks nice!

Bonus points for adding the tests for the Account request method itself. You can improve it further after the setFinishedWithError ctor with an invalid creation properties instance. The PCR should fail with INVALID_ARGUMENT in that case.

Please ping me after doing the adjustments from this and the previous comment, and I'll merge.

I updated the branch again and I found out why it was causing SIGSEGV.
In account-channel-dispatcher.cpp the method TestAccountChannelDispatcher::testPCR is always checking if channelRequest()->channel().isNull(), but if the constructor by errorName and errorMessage of pendingChannelRequest is used, then channelRequest() is null...
I fixed it checking if channelRequest().isNull() before doing other stuff, but I'm not sure if I should set the channel somehow...

(In reply to comment #17)
> I updated the branch again and I found out why it was causing SIGSEGV.
> In account-channel-dispatcher.cpp the method
> TestAccountChannelDispatcher::testPCR is always checking if
> channelRequest()->channel().isNull(), but if the constructor by errorName and
> errorMessage of pendingChannelRequest is used, then channelRequest() is null...

Makes sense.

> I fixed it checking if channelRequest().isNull() before doing other stuff, but
> I'm not sure if I should set the channel somehow...

No, this was purely a limitation of the test. Actual operation of the API, even with supposedly correct parameters, can result in a null ChannelRequest if the initial D-Bus call to the ChannelDispatcher fails (as that call would return the object path of the ChannelRequest). It's perfectly consistent that the early-out case for invalid parameters gives a null channel request as well (because there isn't one).

I'm merging this now. Will be in 0.7.1. Thanks again for your work!