[Bug 37034] Telepathy-Qt4 doesn't set the URI property for file transfers.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #5 from Daniele E. Domenichelli <daniele.domenichelli at gmail.com> 2011-05-20 17:17:10 PDT ---
(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...)

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